Add advisory lock before creating label in create/merge operation

[mdianjun]

When merging nodes or edges concurrently, there is label with the same name created by different session.
However, checking whether the label exists cannot be guaranteed consistency. This commit add an advisory lock
to ensure creating label is serial.

[mdianjun]

@MuhammadTahaNaveed PTAL, thanks.

[jrgemignani]

@MuhammadTahaNaveed Can you check if this is a new CI issue?

[jrgemignani]

@mdianjun Can you rebase (against the master) and force push this PR? We had to fix the CI that was causing build issues.

[mdianjun]

@mdianjun Can you rebase (against the master) and force push this PR? We had to fix the CI that was causing build issues.

Have rebased it.

[jrgemignani]

When merging nodes or edges concurrently, there is label with the same name created by different session. However, checking whether the label exists cannot be guaranteed consistency. This commit add an advisory lock to ensure creating label is serial.

@mdianjun As this fixes a purported issue, can you provide an example showing this issue? Additionally, can you provide regression tests for it?

edit: Why only merge and not create?

[mdianjun]

@mdianjun As this fixes a purported issue, can you provide an example showing this issue? Additionally, can you provide regression tests for it?

edit: Why only merge and not create?

@jrgemignani Create operation should be fixed as well, I missed it. I add a python script in regress test.

[jrgemignani]

@MuhammadTahaNaveed Is this something we need to add/fix? Or,...

[gregfelice]

Thanks for picking this back up, and sorry it sat so long. I dug into it against current master (PG18). Short version: the bug you're fixing is real and still reproduces, all four call sites are covered, and the advisory lock does eliminate the race. But as written it trades the race for a deterministic deadlock, so I can't approve it yet. Details below, including a reproducer for each claim so you can verify locally.

To answer @jrgemignani's earlier question ("is this something we need to fix?") — yes, it still reproduces on master today.

The bug is real (reproduced on master)

10 sessions concurrently creating the same brand-new label:

-- CREATE: 3 of 10 sessions failed
ERROR:  relation "RaceLabelA" already exists

-- MERGE: raw catalog error leaks to the user
ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"

Classic check-then-act TOCTOU between label_exists() and create_label(). One mitigating point worth stating: after the dust settles the catalog is consistent — no duplicate labels, no corruption. So this is spurious errors for concurrent writers, not data loss. Good to fix, but not a corruption emergency.

The blocker: this introduces an ABBA deadlock

The lock is taken per-label, in pattern order, and held to transaction end. Two sessions creating the same labels in opposite order form a lock-ordering cycle:

session 1:  CREATE (:A)-[:R]->(:B)
session 2:  CREATE (:B)-[:R]->(:A)

Measured, same workload both ways:

	master	this PR
opposing-order label creation	intermittent "already exists" (~5/12)	deadlock detected — 6 of 6 pairs, every time

So for any multi-label pattern created in opposing order, this converts an intermittent race into a reliable deadlock — and each one first blocks for deadlock_timeout (1s default) before aborting. That's a regression.

Fix: acquire label locks in a deterministic global order

Collect every label the statement needs to create, sort them into a stable order (by the hashed key, or by name), and take all the locks in that order before creating any of them. Same serialization, no cycle. As it stands each call site locks one label inline in pattern order, which is exactly what produces the ABBA.

The test cannot run in CI, and would not have caught this

regress/sql/concurrent.sql is just \! python3 regress/python/test_concurrent.py. Three problems:

1. It imports psycopg2, which the Build/Regression workflow never installs (only build-essential/libreadline/zlib/flex/bison), so it fails with ModuleNotFoundError — the test is effectively skipped.
2. It connects out-of-band with host/port/user/password defaulting to empty strings, so it can't reach the pg_regress temp instance even if psycopg2 were present.
3. Every thread creates labels in the same order, so it exercises the happy path only — it would pass even with the deadlock bug above unfixed.

A regression test here needs to (a) run under pg_regress without external Python deps, and (b) include the opposing-order case so it actually guards the fix. A TAP test using BackgroundPsql, or an isolation-tester spec, is the idiomatic way to drive concurrent sessions from the regress suite.

Minor

• Two added lines use tabs (\t) for indentation; the file is 4-space throughout. git diff --check flags them.
• SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, key, cpstate->graph_oid, 3) — the 3 is a bare magic number. It's the right idea (PostgreSQL reserves 1 and 2 for user-facing pg_advisory_* locks, so 3 avoids collision), but please give it a named constant and a one-line comment saying exactly that.
• LockAcquire(&tag, ExclusiveLock, false, false) with dontWait=false is what causes the wait-then-deadlock. With deterministic ordering the wait is fine; just flagging that the current signature is load-bearing for the deadlock behaviour.

Housekeeping

The PR was conflicting against master (only the Makefile REGRESS list). I rebased it locally to test, so the conflict is trivial — a rebase + force-push on your side will clear the DIRTY state.

Happy to re-review once the lock ordering is deterministic and the test runs inside pg_regress. The core instinct is right — an advisory lock keyed on (label, graph) is a reasonable way to serialize this — it just needs the ordering and the test to land.