agora inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 3/5] Make archiver process an auxiliary process
5+ messages / 5 participants
[nested] [flat]

* [PATCH 3/5] Make archiver process an auxiliary process
@ 2018-11-07 07:53 Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Kyotaro Horiguchi @ 2018-11-07 07:53 UTC (permalink / raw)

This is a preliminary patch for shared-memory based stats collector.
Archiver process must be a auxiliary process since it uses shared
memory after stats data wes moved onto shared-memory. Make the process
an auxiliary process in order to make it work.
---
 src/backend/bootstrap/bootstrap.c   |  8 +++
 src/backend/postmaster/pgarch.c     | 98 +++++++++----------------------------
 src/backend/postmaster/pgstat.c     |  6 +++
 src/backend/postmaster/postmaster.c | 35 +++++++++----
 src/include/miscadmin.h             |  2 +
 src/include/pgstat.h                |  1 +
 src/include/postmaster/pgarch.h     |  4 +-
 7 files changed, 67 insertions(+), 87 deletions(-)

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 43627ab8f4..7872a2d9d7 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -329,6 +329,9 @@ AuxiliaryProcessMain(int argc, char *argv[])
 			case BgWriterProcess:
 				statmsg = pgstat_get_backend_desc(B_BG_WRITER);
 				break;
+			case ArchiverProcess:
+				statmsg = pgstat_get_backend_desc(B_ARCHIVER);
+				break;
 			case CheckpointerProcess:
 				statmsg = pgstat_get_backend_desc(B_CHECKPOINTER);
 				break;
@@ -456,6 +459,11 @@ AuxiliaryProcessMain(int argc, char *argv[])
 			BackgroundWriterMain();
 			proc_exit(1);		/* should never return */
 
+		case ArchiverProcess:
+			/* don't set signals, archiver has its own agenda */
+			PgArchiverMain();
+			proc_exit(1);		/* should never return */
+
 		case CheckpointerProcess:
 			/* don't set signals, checkpointer has its own agenda */
 			CheckpointerMain();
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index f84f882c4c..4342ebdab4 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -77,7 +77,6 @@
  * Local data
  * ----------
  */
-static time_t last_pgarch_start_time;
 static time_t last_sigterm_time = 0;
 
 /*
@@ -96,7 +95,6 @@ static volatile sig_atomic_t ready_to_stop = false;
 static pid_t pgarch_forkexec(void);
 #endif
 
-NON_EXEC_STATIC void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn();
 static void pgarch_exit(SIGNAL_ARGS);
 static void ArchSigHupHandler(SIGNAL_ARGS);
 static void ArchSigTermHandler(SIGNAL_ARGS);
@@ -114,75 +112,6 @@ static void pgarch_archiveDone(char *xlog);
  * ------------------------------------------------------------
  */
 
-/*
- * pgarch_start
- *
- *	Called from postmaster at startup or after an existing archiver
- *	died.  Attempt to fire up a fresh archiver process.
- *
- *	Returns PID of child process, or 0 if fail.
- *
- *	Note: if fail, we will be called again from the postmaster main loop.
- */
-int
-pgarch_start(void)
-{
-	time_t		curtime;
-	pid_t		pgArchPid;
-
-	/*
-	 * Do nothing if no archiver needed
-	 */
-	if (!XLogArchivingActive())
-		return 0;
-
-	/*
-	 * Do nothing if too soon since last archiver start.  This is a safety
-	 * valve to protect against continuous respawn attempts if the archiver is
-	 * dying immediately at launch. Note that since we will be re-called from
-	 * the postmaster main loop, we will get another chance later.
-	 */
-	curtime = time(NULL);
-	if ((unsigned int) (curtime - last_pgarch_start_time) <
-		(unsigned int) PGARCH_RESTART_INTERVAL)
-		return 0;
-	last_pgarch_start_time = curtime;
-
-#ifdef EXEC_BACKEND
-	switch ((pgArchPid = pgarch_forkexec()))
-#else
-	switch ((pgArchPid = fork_process()))
-#endif
-	{
-		case -1:
-			ereport(LOG,
-					(errmsg("could not fork archiver: %m")));
-			return 0;
-
-#ifndef EXEC_BACKEND
-		case 0:
-			/* in postmaster child ... */
-			InitPostmasterChild();
-
-			/* Close the postmaster's sockets */
-			ClosePostmasterPorts(false);
-
-			/* Drop our connection to postmaster's shared memory, as well */
-			dsm_detach_all();
-			PGSharedMemoryDetach();
-
-			PgArchiverMain(0, NULL);
-			break;
-#endif
-
-		default:
-			return (int) pgArchPid;
-	}
-
-	/* shouldn't get here */
-	return 0;
-}
-
 /* ------------------------------------------------------------
  * Local functions called by archiver follow
  * ------------------------------------------------------------
@@ -222,8 +151,8 @@ pgarch_forkexec(void)
  *	The argc/argv parameters are valid only in EXEC_BACKEND case.  However,
  *	since we don't use 'em, it hardly matters...
  */
-NON_EXEC_STATIC void
-PgArchiverMain(int argc, char *argv[])
+void
+PgArchiverMain(void)
 {
 	/*
 	 * Ignore all signals usually bound to some action in the postmaster,
@@ -255,8 +184,27 @@ PgArchiverMain(int argc, char *argv[])
 static void
 pgarch_exit(SIGNAL_ARGS)
 {
-	/* SIGQUIT means curl up and die ... */
-	exit(1);
+	PG_SETMASK(&BlockSig);
+
+	/*
+	 * We DO NOT want to run proc_exit() callbacks -- we're here because
+	 * shared memory may be corrupted, so we don't want to try to clean up our
+	 * transaction.  Just nail the windows shut and get out of town.  Now that
+	 * there's an atexit callback to prevent third-party code from breaking
+	 * things by calling exit() directly, we have to reset the callbacks
+	 * explicitly to make this work as intended.
+	 */
+	on_exit_reset();
+
+	/*
+	 * Note we do exit(2) not exit(0).  This is to force the postmaster into a
+	 * system reset cycle if some idiot DBA sends a manual SIGQUIT to a random
+	 * backend.  This is necessary precisely because we don't clean up our
+	 * shared memory state.  (The "dead man switch" mechanism in pmsignal.c
+	 * should ensure the postmaster sees this as a crash, too, but no harm in
+	 * being doubly sure.)
+	 */
+	exit(2);
 }
 
 /* SIGHUP signal handler for archiver process */
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b4f2b28b51..f4ec142cab 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2934,6 +2934,9 @@ pgstat_bestart(void)
 			case StartupProcess:
 				lbeentry.st_backendType = B_STARTUP;
 				break;
+			case ArchiverProcess:
+				beentry->st_backendType = B_ARCHIVER;
+				break;
 			case BgWriterProcess:
 				lbeentry.st_backendType = B_BG_WRITER;
 				break;
@@ -4277,6 +4280,9 @@ pgstat_get_backend_desc(BackendType backendType)
 
 	switch (backendType)
 	{
+		case B_ARCHIVER:
+			backendDesc = "archiver";
+			break;
 		case B_AUTOVAC_LAUNCHER:
 			backendDesc = "autovacuum launcher";
 			break;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 3339804be9..0ca0b3024b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -146,7 +146,8 @@
 #define BACKEND_TYPE_AUTOVAC	0x0002	/* autovacuum worker process */
 #define BACKEND_TYPE_WALSND		0x0004	/* walsender process */
 #define BACKEND_TYPE_BGWORKER	0x0008	/* bgworker process */
-#define BACKEND_TYPE_ALL		0x000F	/* OR of all the above */
+#define BACKEND_TYPE_ARCHIVER	0x0010	/* archiver process */
+#define BACKEND_TYPE_ALL		0x001F	/* OR of all the above */
 
 #define BACKEND_TYPE_WORKER		(BACKEND_TYPE_AUTOVAC | BACKEND_TYPE_BGWORKER)
 
@@ -539,6 +540,7 @@ static void ShmemBackendArrayRemove(Backend *bn);
 #endif							/* EXEC_BACKEND */
 
 #define StartupDataBase()		StartChildProcess(StartupProcess)
+#define StartArchiver()			StartChildProcess(ArchiverProcess)
 #define StartBackgroundWriter() StartChildProcess(BgWriterProcess)
 #define StartCheckpointer()		StartChildProcess(CheckpointerProcess)
 #define StartWalWriter()		StartChildProcess(WalWriterProcess)
@@ -1762,7 +1764,7 @@ ServerLoop(void)
 
 		/* If we have lost the archiver, try to start a new one. */
 		if (PgArchPID == 0 && PgArchStartupAllowed())
-			PgArchPID = pgarch_start();
+			PgArchPID = StartArchiver();
 
 		/* If we need to signal the autovacuum launcher, do so now */
 		if (avlauncher_needs_signal)
@@ -2977,7 +2979,7 @@ reaper(SIGNAL_ARGS)
 			if (!IsBinaryUpgrade && AutoVacuumingActive() && AutoVacPID == 0)
 				AutoVacPID = StartAutoVacLauncher();
 			if (PgArchStartupAllowed() && PgArchPID == 0)
-				PgArchPID = pgarch_start();
+				PgArchPID = StartArchiver();
 			if (PgStatPID == 0)
 				PgStatPID = pgstat_start();
 
@@ -3122,10 +3124,8 @@ reaper(SIGNAL_ARGS)
 		{
 			PgArchPID = 0;
 			if (!EXIT_STATUS_0(exitstatus))
-				LogChildExit(LOG, _("archiver process"),
-							 pid, exitstatus);
-			if (PgArchStartupAllowed())
-				PgArchPID = pgarch_start();
+				HandleChildCrash(pid, exitstatus,
+								 _("archiver process"));
 			continue;
 		}
 
@@ -3371,7 +3371,7 @@ CleanupBackend(int pid,
 
 /*
  * HandleChildCrash -- cleanup after failed backend, bgwriter, checkpointer,
- * walwriter, autovacuum, or background worker.
+ * walwriter, autovacuum, archiver or background worker.
  *
  * The objectives here are to clean up our local state about the child
  * process, and to signal all other remaining children to quickdie.
@@ -3576,6 +3576,18 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
 		signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT));
 	}
 
+	/* Take care of the archiver too */
+	if (pid == PgArchPID)
+		PgArchPID = 0;
+	else if (PgArchPID != 0 && take_action)
+	{
+		ereport(DEBUG2,
+				(errmsg_internal("sending %s to process %d",
+								 (SendStop ? "SIGSTOP" : "SIGQUIT"),
+								 (int) PgArchPID)));
+		signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT));
+	}
+
 	/*
 	 * Force a power-cycle of the pgarch process too.  (This isn't absolutely
 	 * necessary, but it seems like a good idea for robustness, and it
@@ -3848,6 +3860,7 @@ PostmasterStateMachine(void)
 			Assert(CheckpointerPID == 0);
 			Assert(WalWriterPID == 0);
 			Assert(AutoVacPID == 0);
+			Assert(PgArchPID == 0);
 			/* syslogger is not considered here */
 			pmState = PM_NO_CHILDREN;
 		}
@@ -5117,7 +5130,7 @@ sigusr1_handler(SIGNAL_ARGS)
 		 */
 		Assert(PgArchPID == 0);
 		if (XLogArchivingAlways())
-			PgArchPID = pgarch_start();
+			PgArchPID = StartArchiver();
 
 		/*
 		 * If we aren't planning to enter hot standby mode later, treat
@@ -5400,6 +5413,10 @@ StartChildProcess(AuxProcType type)
 				ereport(LOG,
 						(errmsg("could not fork startup process: %m")));
 				break;
+			case ArchiverProcess:
+				ereport(LOG,
+						(errmsg("could not fork archiver process: %m")));
+				break;
 			case BgWriterProcess:
 				ereport(LOG,
 						(errmsg("could not fork background writer process: %m")));
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 61a24c2e3c..0b49b63327 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -399,6 +399,7 @@ typedef enum
 	BootstrapProcess,
 	StartupProcess,
 	BgWriterProcess,
+	ArchiverProcess,
 	CheckpointerProcess,
 	WalWriterProcess,
 	WalReceiverProcess,
@@ -411,6 +412,7 @@ extern AuxProcType MyAuxProcType;
 #define AmBootstrapProcess()		(MyAuxProcType == BootstrapProcess)
 #define AmStartupProcess()			(MyAuxProcType == StartupProcess)
 #define AmBackgroundWriterProcess() (MyAuxProcType == BgWriterProcess)
+#define AmArchiverProcess()			(MyAuxProcType == ArchiverProcess)
 #define AmCheckpointerProcess()		(MyAuxProcType == CheckpointerProcess)
 #define AmWalWriterProcess()		(MyAuxProcType == WalWriterProcess)
 #define AmWalReceiverProcess()		(MyAuxProcType == WalReceiverProcess)
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 0a3ad3a188..b3f00e1943 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -719,6 +719,7 @@ typedef struct PgStat_GlobalStats
  */
 typedef enum BackendType
 {
+	B_ARCHIVER,
 	B_AUTOVAC_LAUNCHER,
 	B_AUTOVAC_WORKER,
 	B_BACKEND,
diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h
index 2474eac26a..88f16863d4 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -32,8 +32,6 @@
  */
 extern int	pgarch_start(void);
 
-#ifdef EXEC_BACKEND
-extern void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn();
-#endif
+extern void PgArchiverMain(void) pg_attribute_noreturn();
 
 #endif							/* _PGARCH_H */
-- 
2.16.3


----Next_Part(Thu_Jul_11_16_40_59_2019_397)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v21-0004-Shared-memory-based-stats-collector.patch"



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* [PATCH v1 7/9] regress: Check for postgres startup completion more often
@ 2023-08-07 23:51 Andres Freund <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Andres Freund @ 2023-08-07 23:51 UTC (permalink / raw)

Previously pg_regress.c only checked whether the server started up once a
second - in most cases startup is much faster though. Use the same interval as
pg_ctl does.
---
 src/test/regress/pg_regress.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 407e3915cec..46af1fddbdb 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -75,6 +75,9 @@ const char *pretty_diff_opts = "-w -U3";
  */
 #define TESTNAME_WIDTH 36
 
+/* how often to recheck if postgres startup completed */
+#define WAITS_PER_SEC	10
+
 typedef enum TAPtype
 {
 	DIAG = 0,
@@ -2499,7 +2502,7 @@ regression_main(int argc, char *argv[],
 		else
 			wait_seconds = 60;
 
-		for (i = 0; i < wait_seconds; i++)
+		for (i = 0; i < wait_seconds * WAITS_PER_SEC; i++)
 		{
 			/* Done if psql succeeds */
 			fflush(NULL);
@@ -2519,7 +2522,7 @@ regression_main(int argc, char *argv[],
 					 outputdir);
 			}
 
-			pg_usleep(1000000L);
+			pg_usleep(1000000L / WAITS_PER_SEC);
 		}
 		if (i >= wait_seconds)
 		{
-- 
2.38.0


--uu4yojthqnm7ulmd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0008-ci-Don-t-specify-amount-of-memory.patch"



^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* [PATCH v1] Fix other cases of literal 0 instead of InvalidXLogRecPtr assignment
@ 2026-01-30 07:50 Bertrand Drouvot <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Bertrand Drouvot @ 2026-01-30 07:50 UTC (permalink / raw)

Those ones were missed in ec317440716.

Author: Bertrand Drouvot <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 src/backend/access/transam/xact.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
 100.0% src/backend/access/transam/

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index eba4f063168..7e3dc3f9386 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1573,7 +1573,7 @@ RecordTransactionCommit(void)
 	XactLastCommitEnd = XactLastRecEnd;
 
 	/* Reset XactLastRecEnd until the next transaction writes something */
-	XactLastRecEnd = 0;
+	XactLastRecEnd = InvalidXLogRecPtr;
 cleanup:
 	/* Clean up local data */
 	if (rels)
@@ -1787,7 +1787,7 @@ RecordTransactionAbort(bool isSubXact)
 	{
 		/* Reset XactLastRecEnd until the next transaction writes something */
 		if (!isSubXact)
-			XactLastRecEnd = 0;
+			XactLastRecEnd = InvalidXLogRecPtr;
 		return InvalidTransactionId;
 	}
 
@@ -1879,7 +1879,7 @@ RecordTransactionAbort(bool isSubXact)
 
 	/* Reset XactLastRecEnd until the next transaction writes something */
 	if (!isSubXact)
-		XactLastRecEnd = 0;
+		XactLastRecEnd = InvalidXLogRecPtr;
 
 	/* And clean up local data */
 	if (rels)
@@ -2701,7 +2701,7 @@ PrepareTransaction(void)
 	 */
 
 	/* Reset XactLastRecEnd until the next transaction writes something */
-	XactLastRecEnd = 0;
+	XactLastRecEnd = InvalidXLogRecPtr;
 
 	/*
 	 * Transfer our locks to a dummy PGPROC.  This has to be done before
-- 
2.34.1


--31r/W+W8Qcw0VOW+--






^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure
@ 2026-07-03 14:45 Peter Eisentraut <[email protected]>
  2026-07-06 11:36 ` Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure Ashutosh Bapat <[email protected]>
  0 siblings, 1 reply; 5+ messages in thread

From: Peter Eisentraut @ 2026-07-03 14:45 UTC (permalink / raw)
  To: Ashutosh Bapat <[email protected]>; +Cc: SATYANARAYANA NARLAPURAM <[email protected]>; PostgreSQL Hackers <[email protected]>

On 19.06.26 14:05, Ashutosh Bapat wrote:
> On Tue, May 12, 2026 at 12:31 PM Ashutosh Bapat
> <[email protected]> wrote:
>>
>> On Tue, May 12, 2026 at 7:05 AM Ashutosh Bapat
>> <[email protected]> wrote:
>>>
>>> On Mon, May 4, 2026 at 8:16 PM Peter Eisentraut <[email protected]> wrote:
>>>>
>>>> On 28.04.26 17:02, Ashutosh Bapat wrote:
>>>>> We are looking up element label catalogs twice in this patch - first
>>>>> to find the label to be dropped and then to find the number of labels
>>>>> associated with the given element. I combined these two into a single
>>>>> while loop.
>>>>
>>>> That looks okay, but I think the names of the local variables are now a
>>>> bit off.  I would expect elrel and elscan to refer to
>>>> pg_propgraph_element, not pg_propgraph_element_label.  Maybe use
>>>> ellabelrel etc.
>>>
>>> Done.
>>>
>>>>
>>>> Also, I think this code needs to think a bit about locking to handle the
>>>> situation where more than one DROP LABEL operation happens concurrently.
>>>>
>>>
>>> AlterPropGraph already takes ShareRowExclusiveLock at the beginning so
>>> only one label can be dropped at a time. I have added an isolation
>>> test to test the scenario. We could further add some more tests to
>>> make sure that properties can not be added to a label being dropped,
>>> adding label to an element being dropped, adding label to an element
>>> being added etc. Would that be an overkill?
>>
>> Here's the patchset without the extra tests.
> 
> I decided to go ahead and added those extra tests as a separate patch.
> They don't cover all the possible concurrent modifications, but the
> ones which may render a property or a label orphan.
> 
> 0001 - patch to avoid dropping last label from an element
> 0002 - patch to test concurrent ALTER PROPERTY GRAPH
> 0003 - patch from [1] which would conflict with 0001, if not included here.
> 
> [1] https://www.postgresql.org/message-id/[email protected]

I have committed 0001 and 0003.  You are right that the lock prevents 
concurrent modifications.  This is pretty straightforward, so I have 
omitted the patch 0002 with the new tests.  I have added a brief comment 
where the lock is taken.






^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure
  2026-07-03 14:45 Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure Peter Eisentraut <[email protected]>
@ 2026-07-06 11:36 ` Ashutosh Bapat <[email protected]>
  0 siblings, 0 replies; 5+ messages in thread

From: Ashutosh Bapat @ 2026-07-06 11:36 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: SATYANARAYANA NARLAPURAM <[email protected]>; PostgreSQL Hackers <[email protected]>

On Fri, Jul 3, 2026 at 8:15 PM Peter Eisentraut <[email protected]> wrote:
>
> On 19.06.26 14:05, Ashutosh Bapat wrote:
> > On Tue, May 12, 2026 at 12:31 PM Ashutosh Bapat
> > <[email protected]> wrote:
> >>
> >> On Tue, May 12, 2026 at 7:05 AM Ashutosh Bapat
> >> <[email protected]> wrote:
> >>>
> >>> On Mon, May 4, 2026 at 8:16 PM Peter Eisentraut <[email protected]> wrote:
> >>>>
> >>>> On 28.04.26 17:02, Ashutosh Bapat wrote:
> >>>>> We are looking up element label catalogs twice in this patch - first
> >>>>> to find the label to be dropped and then to find the number of labels
> >>>>> associated with the given element. I combined these two into a single
> >>>>> while loop.
> >>>>
> >>>> That looks okay, but I think the names of the local variables are now a
> >>>> bit off.  I would expect elrel and elscan to refer to
> >>>> pg_propgraph_element, not pg_propgraph_element_label.  Maybe use
> >>>> ellabelrel etc.
> >>>
> >>> Done.
> >>>
> >>>>
> >>>> Also, I think this code needs to think a bit about locking to handle the
> >>>> situation where more than one DROP LABEL operation happens concurrently.
> >>>>
> >>>
> >>> AlterPropGraph already takes ShareRowExclusiveLock at the beginning so
> >>> only one label can be dropped at a time. I have added an isolation
> >>> test to test the scenario. We could further add some more tests to
> >>> make sure that properties can not be added to a label being dropped,
> >>> adding label to an element being dropped, adding label to an element
> >>> being added etc. Would that be an overkill?
> >>
> >> Here's the patchset without the extra tests.
> >
> > I decided to go ahead and added those extra tests as a separate patch.
> > They don't cover all the possible concurrent modifications, but the
> > ones which may render a property or a label orphan.
> >
> > 0001 - patch to avoid dropping last label from an element
> > 0002 - patch to test concurrent ALTER PROPERTY GRAPH
> > 0003 - patch from [1] which would conflict with 0001, if not included here.
> >
> > [1] https://www.postgresql.org/message-id/[email protected]
>
> I have committed 0001 and 0003.  You are right that the lock prevents
> concurrent modifications.  This is pretty straightforward, so I have
> omitted the patch 0002 with the new tests.  I have added a brief comment
> where the lock is taken.
>

Thanks. Works for me.

-- 
Best Wishes,
Ashutosh Bapat






^ permalink  raw  reply  [nested|flat] 5+ messages in thread


end of thread, other threads:[~2026-07-06 11:36 UTC | newest]

Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 07:53 [PATCH 3/5] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]>
2023-08-07 23:51 [PATCH v1 7/9] regress: Check for postgres startup completion more often Andres Freund <[email protected]>
2026-01-30 07:50 [PATCH v1] Fix other cases of literal 0 instead of InvalidXLogRecPtr assignment Bertrand Drouvot <[email protected]>
2026-07-03 14:45 Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure Peter Eisentraut <[email protected]>
2026-07-06 11:36 ` Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure Ashutosh Bapat <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox