Fix an incorrect assertion condition in mdwritev().

2024-05-25 Thread Xing Guo 
Hi hackers,

In commit 4908c58[^1], a vectored variant of smgrwrite() is added and
the assertion condition in mdwritev() no longer matches the comment.
This patch helps fix it.

[^1]: 
https://github.com/postgres/postgres/commit/4908c5872059c409aa647bcde758dfeffe07996e

Best Regards,
Xing
From c77d1810ac6f13e9b58da5cd3ded5e47d44d03af Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Sat, 25 May 2024 23:36:29 +0800
Subject: [PATCH v1] Fix an incorrect assertion in mdwritev().

In commit 4908c58[^1], we added smgrwritev() which is a vectored variant
of smgrwrite().  Since mdwritev() is to be used only for updating
already-existing blocks of a relation, the assertion condition shall be
updated accordingly.

[^1]: https://github.com/postgres/postgres/commit/4908c5872059c409aa647bcde758dfeffe07996e
---
 src/backend/storage/smgr/md.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index bf0f3ca76d..2abb5133a6 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -930,7 +930,7 @@ mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 {
 	/* This assert is too expensive to have on normally ... */
 #ifdef CHECK_WRITE_VS_EXTEND
-	Assert(blocknum < mdnblocks(reln, forknum));
+	Assert(blocknum + nblocks <= mdnblocks(reln, forknum));
 #endif
 
 	while (nblocks > 0)
-- 
2.45.1

Re: Set appropriate processing mode for auxiliary processes.

2024-05-09 Thread Xing Guo 
On Thu, May 9, 2024 at 11:19 PM Tom Lane  wrote:
>
> Heikki Linnakangas  writes:
> > At first I was sure this was introduced by my refactorings in v17, but
> > in fact it's been like this forever. I agree that InitProcessing makes
> > much more sense. The ProcessingMode variable is initialized to
> > InitProcessing, so I think we can simply remove that line from
> > AuxiliaryProcessMainCommon(). There are existing
> > "SetProcessingMode(InitProcessing)" calls in other Main functions too
> > (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can
> > also be removed.
>
> This only works if the postmaster can be trusted never to change the
> variable; else children could inherit some other value via fork().
> In that connection, it seems a bit scary that postmaster.c contains a
> couple of calls "SetProcessingMode(NormalProcessing)".  It looks like
> they are in functions that should only be executed by child processes,
> but should we try to move them somewhere else?

After checking calls to "SetProcessingMode(NormalProcessing)" in the
postmaster.c, they are used in background worker specific functions
(BackgroundWorkerInitializeConnectionByOid and
BackgroundWorkerInitializeConnection). So I think it's a good idea to
move these functions to bgworker.c. Then, we can get rid of calling
"SetProcessingMode(NormalProcessing)" in postmaster.c.

I also noticed that there's an unnecessary call to
"BackgroundWorkerInitializeConnection" in worker_spi.c (The worker_spi
launcher has set the dboid correctly).

Best Regards,
Xing.
From 4dbe54ef746203740202e181f731b7e08bb6e4ea Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Fri, 10 May 2024 10:34:09 +0800
Subject: [PATCH v4 1/3] Move bgworker specific logic to bgworker.c.

---
 src/backend/postmaster/bgworker.c   | 83 +
 src/backend/postmaster/postmaster.c | 83 -
 2 files changed, 83 insertions(+), 83 deletions(-)

diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cf64a4beb2..0ab64ae3ec 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -1240,6 +1240,89 @@ TerminateBackgroundWorker(BackgroundWorkerHandle *handle)
 		SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE);
 }
 
+/*
+ * Connect background worker to a database.
+ */
+void
+BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
+{
+	BackgroundWorker *worker = MyBgworkerEntry;
+	bits32		init_flags = 0; /* never honor session_preload_libraries */
+
+	/* ignore datallowconn? */
+	if (flags & BGWORKER_BYPASS_ALLOWCONN)
+		init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+	/* ignore rolcanlogin? */
+	if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+		init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
+
+	/* XXX is this the right errcode? */
+	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
+		ereport(FATAL,
+(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("database connection requirement not indicated during registration")));
+
+	InitPostgres(dbname, InvalidOid,	/* database to connect to */
+ username, InvalidOid,	/* role to connect as */
+ init_flags,
+ NULL);			/* no out_dbname */
+
+	/* it had better not gotten out of "init" mode yet */
+	if (!IsInitProcessingMode())
+		ereport(ERROR,
+(errmsg("invalid processing mode in background worker")));
+	SetProcessingMode(NormalProcessing);
+}
+
+/*
+ * Connect background worker to a database using OIDs.
+ */
+void
+BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
+{
+	BackgroundWorker *worker = MyBgworkerEntry;
+	bits32		init_flags = 0; /* never honor session_preload_libraries */
+
+	/* ignore datallowconn? */
+	if (flags & BGWORKER_BYPASS_ALLOWCONN)
+		init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+	/* ignore rolcanlogin? */
+	if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+		init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
+
+	/* XXX is this the right errcode? */
+	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
+		ereport(FATAL,
+(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("database connection requirement not indicated during registration")));
+
+	InitPostgres(NULL, dboid,	/* database to connect to */
+ NULL, useroid, /* role to connect as */
+ init_flags,
+ NULL);			/* no out_dbname */
+
+	/* it had better not gotten out of "init" mode yet */
+	if (!IsInitProcessingMode())
+		ereport(ERROR,
+(errmsg("invalid processing mode in background worker")));
+	SetProcessingMode(NormalProcessing);
+}
+
+/*
+ * Block/unblock signals in a background worker
+ */
+void
+BackgroundWorkerBlockSignals(void)
+{
+	sigprocmask(SIG_SETMASK, , NULL);
+}
+
+void
+BackgroundWorkerUnblockSignals(

Re: Set appropriate processing mode for auxiliary processes.

2024-05-09 Thread Xing Guo 
Sorry, forget to add an assertion to guard our codes in my previous patch.
From 4f2d70fb27e3ff23b8572c5e679c791daa1f6665 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Thu, 9 May 2024 20:57:48 +0800
Subject: [PATCH v3] Remove redundant SetProcessingMode(InitProcessing) calls.

After several refactoring iterations, auxiliary processes are no longer
initialized from the bootstrapper. Using the InitProcessing mode for
initializing auxiliary processes is more appropriate. Since the global
variable Mode is initialized to InitProcessing, we can remove redundant
calls of SetProcessingMode(InitProcessing).
---
 src/backend/postmaster/autovacuum.c| 4 ++--
 src/backend/postmaster/auxprocess.c| 3 ++-
 src/backend/postmaster/bgworker.c  | 2 +-
 src/backend/replication/logical/slotsync.c | 2 +-
 src/backend/tcop/postgres.c| 2 +-
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..928754b51c 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -380,7 +380,7 @@ AutoVacLauncherMain(char *startup_data, size_t startup_data_len)
 	if (PostAuthDelay)
 		pg_usleep(PostAuthDelay * 100L);
 
-	SetProcessingMode(InitProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
 
 	/*
 	 * Set up signal handlers.  We operate on databases much like a regular
@@ -1373,7 +1373,7 @@ AutoVacWorkerMain(char *startup_data, size_t startup_data_len)
 	MyBackendType = B_AUTOVAC_WORKER;
 	init_ps_display(NULL);
 
-	SetProcessingMode(InitProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
 
 	/*
 	 * Set up signal handlers.  We operate on databases much like a regular
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 78f4263eeb..b21eae7136 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -54,7 +54,8 @@ AuxiliaryProcessMainCommon(void)
 
 	init_ps_display(NULL);
 
-	SetProcessingMode(BootstrapProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
+
 	IgnoreSystemIndexes = true;
 
 	/*
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cf64a4beb2..dc20a25345 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -746,7 +746,7 @@ BackgroundWorkerMain(char *startup_data, size_t startup_data_len)
 	MyBackendType = B_BG_WORKER;
 	init_ps_display(worker->bgw_name);
 
-	SetProcessingMode(InitProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
 
 	/* Apply PostAuthDelay */
 	if (PostAuthDelay > 0)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index f1f44d41ef..2653ce0342 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1342,7 +1342,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len)
 
 	init_ps_display(NULL);
 
-	SetProcessingMode(InitProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
 
 	/*
 	 * Create a per-backend PGPROC struct in shared memory.  We must do this
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2dff28afce..b9fee13612 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4161,7 +4161,7 @@ PostgresMain(const char *dbname, const char *username)
 	Assert(dbname != NULL);
 	Assert(username != NULL);
 
-	SetProcessingMode(InitProcessing);
+	Assert(GetProcessingMode() == InitProcessing);
 
 	/*
 	 * Set up signal handlers.  (InitPostmasterChild or InitStandaloneProcess
-- 
2.45.0

Re: Set appropriate processing mode for auxiliary processes.

2024-05-09 Thread Xing Guo 
On Thu, May 9, 2024 at 10:13 PM Heikki Linnakangas  wrote:
>
> On 09/05/2024 16:12, Xing Guo wrote:
> > Hi hackers,
> >
> > After several refactoring iterations, auxiliary processes are no
> > longer initialized from the bootstrapper. I think using the
> > InitProcessing mode for initializing auxiliary processes is more
> > appropriate.
>
> At first I was sure this was introduced by my refactorings in v17, but
> in fact it's been like this forever. I agree that InitProcessing makes
> much more sense. The ProcessingMode variable is initialized to
> InitProcessing, so I think we can simply remove that line from
> AuxiliaryProcessMainCommon(). There are existing
> "SetProcessingMode(InitProcessing)" calls in other Main functions too
> (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can
> also be removed.

Good catch! I agree with you.

Best Regards,
Xing.
From 210e97c88fd49853d2a6304763ef7e8ad65e1b79 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Thu, 9 May 2024 20:57:48 +0800
Subject: [PATCH v2] Remove redundant SetProcessingMode(InitProcessing) calls.

After several refactoring iterations, auxiliary processes are no longer
initialized from the bootstrapper. Using the InitProcessing mode for
initializing auxiliary processes is more appropriate. Since the global
variable Mode is initialized to InitProcessing, we can remove redundant
calls of SetProcessingMode(InitProcessing).
---
 src/backend/postmaster/autovacuum.c| 4 
 src/backend/postmaster/auxprocess.c| 1 -
 src/backend/postmaster/bgworker.c  | 2 --
 src/backend/replication/logical/slotsync.c | 2 --
 src/backend/tcop/postgres.c| 2 --
 5 files changed, 11 deletions(-)

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..5a1cfabf19 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -380,8 +380,6 @@ AutoVacLauncherMain(char *startup_data, size_t startup_data_len)
 	if (PostAuthDelay)
 		pg_usleep(PostAuthDelay * 100L);
 
-	SetProcessingMode(InitProcessing);
-
 	/*
 	 * Set up signal handlers.  We operate on databases much like a regular
 	 * backend, so we use the same signal handling.  See equivalent code in
@@ -1373,8 +1371,6 @@ AutoVacWorkerMain(char *startup_data, size_t startup_data_len)
 	MyBackendType = B_AUTOVAC_WORKER;
 	init_ps_display(NULL);
 
-	SetProcessingMode(InitProcessing);
-
 	/*
 	 * Set up signal handlers.  We operate on databases much like a regular
 	 * backend, so we use the same signal handling.  See equivalent code in
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 78f4263eeb..0faf7df0b6 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -54,7 +54,6 @@ AuxiliaryProcessMainCommon(void)
 
 	init_ps_display(NULL);
 
-	SetProcessingMode(BootstrapProcessing);
 	IgnoreSystemIndexes = true;
 
 	/*
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cf64a4beb2..5bbe02bbc3 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -746,8 +746,6 @@ BackgroundWorkerMain(char *startup_data, size_t startup_data_len)
 	MyBackendType = B_BG_WORKER;
 	init_ps_display(worker->bgw_name);
 
-	SetProcessingMode(InitProcessing);
-
 	/* Apply PostAuthDelay */
 	if (PostAuthDelay > 0)
 		pg_usleep(PostAuthDelay * 100L);
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index f1f44d41ef..72857694a5 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1342,8 +1342,6 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len)
 
 	init_ps_display(NULL);
 
-	SetProcessingMode(InitProcessing);
-
 	/*
 	 * Create a per-backend PGPROC struct in shared memory.  We must do this
 	 * before we access any shared memory.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2dff28afce..4b2de1dc70 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4161,8 +4161,6 @@ PostgresMain(const char *dbname, const char *username)
 	Assert(dbname != NULL);
 	Assert(username != NULL);
 
-	SetProcessingMode(InitProcessing);
-
 	/*
 	 * Set up signal handlers.  (InitPostmasterChild or InitStandaloneProcess
 	 * has already set up BlockSig and made that the active signal mask.)
-- 
2.45.0

Set appropriate processing mode for auxiliary processes.

2024-05-09 Thread Xing Guo 
Hi hackers,

After several refactoring iterations, auxiliary processes are no
longer initialized from the bootstrapper. I think using the
InitProcessing mode for initializing auxiliary processes is more
appropriate.

Best Regards,
Xing
From 50fb1ccc7259e0ce8512dee236cbfe11635c15fc Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Thu, 9 May 2024 20:57:48 +0800
Subject: [PATCH v1] Set appropriate processing mode for auxiliary processes.

After several refactoring iterations, auxiliary processes are no longer
initialized from the bootstrapper. Using the InitProcessing mode for
initializing auxiliary processes is more appropriate.
---
 src/backend/postmaster/auxprocess.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 78f4263eeb..ca4bfa2ed5 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -54,7 +54,7 @@ AuxiliaryProcessMainCommon(void)
 
 	init_ps_display(NULL);
 
-	SetProcessingMode(BootstrapProcessing);
+	SetProcessingMode(InitProcessing);
 	IgnoreSystemIndexes = true;
 
 	/*
-- 
2.45.0

Re: Remove deprecated header file resowner_private.h.

2024-04-20 Thread Xing Guo 
On Sat, Apr 20, 2024 at 5:03 PM Michael Paquier  wrote:
>
> On Sat, Apr 20, 2024 at 11:54:29AM +0900, Michael Paquier wrote:
> > Will clean up.
>
> While looking at the whole, I've noticed that this has been mentioned
> here by Andres, but the committed patch did not get the call:
> https://www.postgresql.org/message-id/20231117204441.7ff37sw53udg3...@awork3.anarazel.de

You're right. I didn't go through the original thread carefully.
Thanks for the fix.

> --
> Michael

Remove deprecated header file resowner_private.h.

2024-04-19 Thread Xing Guo 
Hi hackers,

I noticed that the header file resowner_private.h is deprecated and no
longer useful after commit b8bff07[^1]. We should remove it.

[^1]: 
https://github.com/postgres/postgres/commit/b8bff07daa85c837a2747b4d35cd5a27e73fb7b2

Best Regards,
Xing
From ab13ca575e92f26a4b936c0d4fa92df23865c2d4 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Sat, 20 Apr 2024 10:00:23 +0800
Subject: [PATCH v1] Remove deprecated header file.

The resowner_private.h is deprecated after b8bff07.
---
 src/include/utils/resowner_private.h | 117 ---
 1 file changed, 117 deletions(-)
 delete mode 100644 src/include/utils/resowner_private.h

diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.h
deleted file mode 100644
index 02b6ed2f2d..00
--- a/src/include/utils/resowner_private.h
+++ /dev/null
@@ -1,117 +0,0 @@
-/*-
- *
- * resowner_private.h
- *	  POSTGRES resource owner private definitions.
- *
- * See utils/resowner/README for more info.
- *
- *
- * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * src/include/utils/resowner_private.h
- *
- *-
- */
-#ifndef RESOWNER_PRIVATE_H
-#define RESOWNER_PRIVATE_H
-
-#include "storage/dsm.h"
-#include "storage/fd.h"
-#include "storage/lock.h"
-#include "utils/catcache.h"
-#include "utils/plancache.h"
-#include "utils/resowner.h"
-#include "utils/snapshot.h"
-
-
-/* support for buffer refcount management */
-extern void ResourceOwnerEnlargeBuffers(ResourceOwner owner);
-extern void ResourceOwnerRememberBuffer(ResourceOwner owner, Buffer buffer);
-extern void ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer);
-
-/* support for IO-in-progress management */
-extern void ResourceOwnerEnlargeBufferIOs(ResourceOwner owner);
-extern void ResourceOwnerRememberBufferIO(ResourceOwner owner, Buffer buffer);
-extern void ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer);
-
-/* support for local lock management */
-extern void ResourceOwnerRememberLock(ResourceOwner owner, LOCALLOCK *locallock);
-extern void ResourceOwnerForgetLock(ResourceOwner owner, LOCALLOCK *locallock);
-
-/* support for catcache refcount management */
-extern void ResourceOwnerEnlargeCatCacheRefs(ResourceOwner owner);
-extern void ResourceOwnerRememberCatCacheRef(ResourceOwner owner,
-			 HeapTuple tuple);
-extern void ResourceOwnerForgetCatCacheRef(ResourceOwner owner,
-		   HeapTuple tuple);
-extern void ResourceOwnerEnlargeCatCacheListRefs(ResourceOwner owner);
-extern void ResourceOwnerRememberCatCacheListRef(ResourceOwner owner,
- CatCList *list);
-extern void ResourceOwnerForgetCatCacheListRef(ResourceOwner owner,
-			   CatCList *list);
-
-/* support for relcache refcount management */
-extern void ResourceOwnerEnlargeRelationRefs(ResourceOwner owner);
-extern void ResourceOwnerRememberRelationRef(ResourceOwner owner,
-			 Relation rel);
-extern void ResourceOwnerForgetRelationRef(ResourceOwner owner,
-		   Relation rel);
-
-/* support for plancache refcount management */
-extern void ResourceOwnerEnlargePlanCacheRefs(ResourceOwner owner);
-extern void ResourceOwnerRememberPlanCacheRef(ResourceOwner owner,
-			  CachedPlan *plan);
-extern void ResourceOwnerForgetPlanCacheRef(ResourceOwner owner,
-			CachedPlan *plan);
-
-/* support for tupledesc refcount management */
-extern void ResourceOwnerEnlargeTupleDescs(ResourceOwner owner);
-extern void ResourceOwnerRememberTupleDesc(ResourceOwner owner,
-		   TupleDesc tupdesc);
-extern void ResourceOwnerForgetTupleDesc(ResourceOwner owner,
-		 TupleDesc tupdesc);
-
-/* support for snapshot refcount management */
-extern void ResourceOwnerEnlargeSnapshots(ResourceOwner owner);
-extern void ResourceOwnerRememberSnapshot(ResourceOwner owner,
-		  Snapshot snapshot);
-extern void ResourceOwnerForgetSnapshot(ResourceOwner owner,
-		Snapshot snapshot);
-
-/* support for temporary file management */
-extern void ResourceOwnerEnlargeFiles(ResourceOwner owner);
-extern void ResourceOwnerRememberFile(ResourceOwner owner,
-	  File file);
-extern void ResourceOwnerForgetFile(ResourceOwner owner,
-	File file);
-
-/* support for dynamic shared memory management */
-extern void ResourceOwnerEnlargeDSMs(ResourceOwner owner);
-extern void ResourceOwnerRememberDSM(ResourceOwner owner,
-	 dsm_segment *);
-extern void ResourceOwnerForgetDSM(ResourceOwner owner,
-   dsm_segment *);
-
-/* support for JITContext management */
-extern void ResourceOwnerEnlargeJIT(ResourceOwner owner);
-extern void ResourceOwnerRememberJIT(ResourceOwner owner,
-

[plpython] Add missing volatile qualifier.

2024-04-01 Thread Xing Guo 
Hi hackers,

I'm playing a toy static analysis checker with PostgreSQL and found a
variable is missing volatile qualifier.

Best Regards,
Xing
From 7084055da64d0433b09e50faff630e551c363f27 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Mon, 1 Apr 2024 21:39:04 +0800
Subject: [PATCH v1] Add missing volatile qualifier.

The object pltargs is modified in the PG_TRY block (line 874) and used
in the PG_CATCH block (line 881) which should be qualified with
volatile.
---
 src/pl/plpython/plpy_exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index e06fde1dd9..cd71082a5f 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -689,7 +689,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			   *pltrelid,
 			   *plttablename,
 			   *plttableschema,
-			   *pltargs = NULL,
+			   *volatile pltargs = NULL,
 			   *pytnew,
 			   *pytold,
 			   *pltdata;
-- 
2.44.0

Re: Disable LLVM bitcode generation with pgxs.mk framework.

2024-03-13 Thread Xing Guo 
> On Wed, Mar 13, 2024 at 2:45 PM Peter Eisentraut  wrote:
>
> On 12.03.24 14:38, Xing Guo wrote:
> > When the PostgreSQL server is configured with --with-llvm, the pgxs.mk
> > framework will generate LLVM bitcode for extensions automatically.
> > Sometimes, I don't want to generate bitcode for some extensions. I can
> > turn off this feature by specifying with_llvm=0 in the make command.
> >
> > ```
> > make with_llvm=0
> > ```
> >
> > Would it be possible to add a new switch in the pgxs.mk framework to
> > allow users to disable this feature? E.g., the Makefile looks like:
> >
> > ```
> > WITH_LLVM=no
> > PG_CONFIG = pg_config
> > PGXS := $(shell $(PG_CONFIG) --pgxs)
> > ```
>
> Can't you just put the very same with_llvm=0 into the makefile?

Ah, you're right. I can set it by overriding that variable.

```
override with_llvm=no
```

Re: Disable LLVM bitcode generation with pgxs.mk framework.

2024-03-12 Thread Xing Guo 
> On Tue, Mar 12, 2024 at 10:40 PM Daniel Gustafsson  wrote:
>
> > On 12 Mar 2024, at 14:38, Xing Guo  wrote:
>
> > Would it be possible to add a new switch in the pgxs.mk framework to
> > allow users to disable this feature?
>
> Something like that doesn't seem unreasonable I think.

Thanks.

I added a new option NO_LLVM_BITCODE to pgxs. I'm not sure if the name
is appropriate.

> --
> Daniel Gustafsson
>
From e19a724fad4949bef9bc4d0f8e58719607d979be Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Wed, 13 Mar 2024 07:56:46 +0800
Subject: [PATCH v1] Add NO_LLVM_BITCODE option to pgxs.

This patch adds a new option NO_LLVM_BITCODE to pgxs to allow user to
disable LLVM bitcode generation completely even if the PostgreSQL
installation is configured with --with-llvm.
---
 doc/src/sgml/extend.sgml | 9 +
 src/makefiles/pgxs.mk| 6 ++
 2 files changed, 15 insertions(+)

diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index 218940ee5c..6fe69746c2 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1719,6 +1719,15 @@ include $(PGXS)
   
  
 
+ 
+  NO_LLVM_BITCODE
+  
+   
+don't generate LLVM bitcode even if the current PostgreSQL installation is configured with --with-llvm
+   
+  
+ 
+
  
   EXTRA_CLEAN
   
diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index 0de3737e78..ec6a3c1f09 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -53,6 +53,8 @@
 # that don't need their build products to be installed
 #   NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
 # tests require special configuration, or don't use pg_regress
+#   NO_LLVM_BITCODE -- don't generate LLVM bitcode even if the current
+# PostgreSQL installation is configured with --with-llvm
 #   EXTRA_CLEAN -- extra files to remove in 'make clean'
 #   PG_CPPFLAGS -- will be prepended to CPPFLAGS
 #   PG_CFLAGS -- will be appended to CFLAGS
@@ -218,6 +220,10 @@ endef
 
 all: $(PROGRAM) $(DATA_built) $(HEADER_allbuilt) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
 
+ifdef NO_LLVM_BITCODE
+with_llvm := no
+endif # NO_LLVM_BITCODE
+
 ifeq ($(with_llvm), yes)
 all: $(addsuffix .bc, $(MODULES)) $(patsubst %.o,%.bc, $(OBJS))
 endif
-- 
2.44.0

Disable LLVM bitcode generation with pgxs.mk framework.

2024-03-12 Thread Xing Guo 
Hi hackers,

When the PostgreSQL server is configured with --with-llvm, the pgxs.mk
framework will generate LLVM bitcode for extensions automatically.
Sometimes, I don't want to generate bitcode for some extensions. I can
turn off this feature by specifying with_llvm=0 in the make command.

```
make with_llvm=0
```

Would it be possible to add a new switch in the pgxs.mk framework to
allow users to disable this feature? E.g., the Makefile looks like:

```
WITH_LLVM=no
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
```

Best Regards,
Xing

Re: Control your disk usage in PG: Introduction to Disk Quota Extension

2024-02-26 Thread Xing Guo 
On Tue, Feb 27, 2024 at 6:38 AM Stephen Frost  wrote:
>
> Greetings,
>
> * Xing Guo (higuox...@gmail.com) wrote:
> > Looks that many hackers are happy with the original patch except that
> > the original patch needs a simple rebase, though it has been 3 years.
>
> I'm not completely against the idea of adding these hooks, but I have to
> say that it's unfortunate to imagine having to use an extension for
> something like quotas as it's really something that would ideally be in
> core.
>
> > Shall we push forward this patch so that it can benefit extensions not
> > only diskquota?
>
> Would be great to hear about other use-cases for these hooks, which
> would also help us be comfortable that these are the right hooks to
> introduce with the correct arguments.  What are the other extensions
> that you're referring to here..?

Sorry, we don't have another concrete extension that utilizes the
smgrcreate / smgrtruncate / smgrdounlinkall hooks right now. But we
have a transparent data encryption extension that utilizes the
smgrread and smgrwrite hooks to mutate the read buffer and write
buffer in place to perform file level encryption and decryption. I
think smgrwrite / smgrread / smgrcreate / smgrtruncate and
smgrdounlinkall hooks fall into the same catagory and these hooks are
different from the recent proposed storage manager API hooks[1].
Probably it is worth starting a new thread and discussing them.

[1]: 
https://www.postgresql.org/message-id/flat/CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK%3DPCNWEMrA%40mail.gmail.com

> Thanks,
>
> Stephen

Best Regards,
Xing

Re: Control your disk usage in PG: Introduction to Disk Quota Extension

2024-02-26 Thread Xing Guo 
On Mon, Feb 26, 2024 at 7:27 PM Daniel Gustafsson  wrote:
>
> > On 1 Jul 2020, at 10:36, Daniel Gustafsson  wrote:
> >
> >> On 27 Mar 2020, at 11:22, Haozhou Wang  wrote:
> >
> >> We rebased this patch with the newest master.
> >
> > This patch now fails to build according to Travis:
> >
> > smgr.c: In function ‘smgrtruncate’:
> > smgr.c:578:47: error: passing argument 4 of ‘smgrtruncate_hook’ makes 
> > integer from pointer without a cast [-Werror=int-conversion]
> >   (*smgrtruncate_hook)(reln, forknum, nforks, nblocks);
> >   ^
> > smgr.c:578:47: note: expected ‘BlockNumber {aka unsigned int}’ but argument 
> > is of type ‘BlockNumber * {aka unsigned int *}’
> >
> >
> > The warning is also present in the Windows build:
> >
> > src/backend/storage/smgr/smgr.c(578): warning C4047: 'function' : 
> > 'BlockNumber' differs in levels of indirection from 'BlockNumber *' 
> > [C:\projects\postgresql\postgres.vcxproj]
> > src/backend/storage/smgr/smgr.c(578): warning C4024: 'smgrtruncate_hook' : 
> > different types for formal and actual parameter 4 
> > [C:\projects\postgresql\postgres.vcxproj]
> > 2 Warning(s)
> >
> > Marking the patch as Waiting for Author.
>
> As the thread has stalled and the above compilation issue hasn't been
> addressed, I'm marking this entry Returned with Feedback.  Feel free to open a
> new entry when there is a fixed patch.

Hi,

Looks that many hackers are happy with the original patch except that
the original patch needs a simple rebase, though it has been 3 years.
Shall we push forward this patch so that it can benefit extensions not
only diskquota?

The attachment is a rebased version of the original patch.

>
> cheers ./daniel
>
>
>
From 6479e1ccc92c53468dc879d33fa1a93f66ae4521 Mon Sep 17 00:00:00 2001
From: Hubert Zhang 
Date: Mon, 26 Feb 2024 19:24:43 +0800
Subject: [PATCH v8] Add smgr hooks to extend the logic of storage management

One example is that these hooks could be used by diskquota extension
to detect heap table change(create/extend/truncate/unlink).

Co-authored-by: Haozhou Wang 
Co-authored-by: Hubert Zhang 
Co-authored-by: Hao Wu 
---
 src/backend/storage/smgr/smgr.c | 21 +
 src/include/storage/smgr.h  | 16 
 2 files changed, 37 insertions(+)

diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index f7f7fe30b6..7992da84ce 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -142,6 +142,15 @@ static dlist_head unpinned_relns;
 static void smgrshutdown(int code, Datum arg);
 static void smgrdestroy(SMgrRelation reln);
 
+/*
+ * Hook for plugins to extend smgr functions.
+ * for example, collect statistics from smgr functions
+ * via recording the active relfilenode information.
+ */
+smgrcreate_hook_type smgrcreate_hook = NULL;
+smgrextend_hook_type smgrextend_hook = NULL;
+smgrtruncate_hook_type smgrtruncate_hook = NULL;
+smgrdounlinkall_hook_type smgrdounlinkall_hook = NULL;
 
 /*
  * smgrinit(), smgrshutdown() -- Initialize or shut down storage
@@ -413,6 +422,9 @@ smgrexists(SMgrRelation reln, ForkNumber forknum)
 void
 smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
 {
+	if (smgrcreate_hook)
+		(*smgrcreate_hook)(reln, forknum, isRedo);
+
 	smgrsw[reln->smgr_which].smgr_create(reln, forknum, isRedo);
 }
 
@@ -471,6 +483,9 @@ smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo)
 	if (nrels == 0)
 		return;
 
+	if (smgrdounlinkall_hook)
+		(*smgrdounlinkall_hook)(rels, nrels, isRedo);
+
 	/*
 	 * Get rid of any remaining buffers for the relations.  bufmgr will just
 	 * drop them without bothering to write the contents.
@@ -538,6 +553,9 @@ void
 smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 		   const void *buffer, bool skipFsync)
 {
+	if (smgrextend_hook)
+		(*smgrextend_hook)(reln, forknum, blocknum, buffer, skipFsync);
+
 	smgrsw[reln->smgr_which].smgr_extend(reln, forknum, blocknum,
 		 buffer, skipFsync);
 
@@ -706,6 +724,9 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb
 {
 	int			i;
 
+	if (smgrtruncate_hook)
+		(*smgrtruncate_hook)(reln, forknum, nforks, nblocks);
+
 	/*
 	 * Get rid of any buffers for the about-to-be-deleted blocks. bufmgr will
 	 * just drop them without bothering to write the contents.
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 2b57addbdb..0ac1ed8f93 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -124,4 +124,20 @@ smgrwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 	smgrwritev(reln, forknum, blocknum, , 1, skipFsync);
 }
 
+/*
+ * Hook for plugins to extend smgr functions.
+ * for example, collect statistics from smgr functions
+ * via recording the active relfilenode information.
+ */
+typedef void (*smgrcreate_hook_type)(SMgrRelation reln, ForkNumber forknum, bool isRedo);
+extern PGDLLIMPORT smgrcreate_hook_type

Make PostgreSQL work with newer libxml2.

2023-12-03 Thread Xing Guo 
Hi hackers,

There is a breaking change of API since the v2.12.0 of libxml2[1][2]. My
compiler complains about incompatible function signatures:


/usr/bin/clang -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla
-Werror=unguarded-availability-new -Wendif-labels -Wmissing
-format-attribute -Wcast-function-type -Wformat-security
-fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-unused-command-line-argument -Wno-compou
nd-token-split-by-macro -Wno-cast-function-type-strict -g -Og -g3 -I. -I.
-I../../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o
xml.o xml.c
xml.c:1199:45: error: incompatible function pointer types passing 'void
(void *, xmlErrorPtr)' (aka 'void (void *, struct _xmlError *)') to
parameter of type '
xmlStructuredErrorFunc' (aka 'void (*)(void *, const struct _xmlError *)')
[-Wincompatible-function-pointer-types]
xmlSetStructuredErrorFunc((void *) errcxt, xml_errorHandler);
   ^~~~
/usr/include/libxml2/libxml/xmlerror.h:898:29: note: passing argument to
parameter 'handler' here
 xmlStructuredErrorFunc handler);
^
xml.c:4806:55: error: incompatible function pointer types passing 'void
(void *, xmlErrorPtr)' (aka 'void (void *, struct _xmlError *)') to
parameter of type '
xmlStructuredErrorFunc' (aka 'void (*)(void *, const struct _xmlError *)')
[-Wincompatible-function-pointer-types]
xmlSetStructuredErrorFunc((void *) xtCxt->xmlerrcxt,
xml_errorHandler);

 ^~~~
/usr/include/libxml2/libxml/xmlerror.h:898:29: note: passing argument to
parameter 'handler' here
 xmlStructuredErrorFunc handler);
^
xml.c:4860:55: error: incompatible function pointer types passing 'void
(void *, xmlErrorPtr)' (aka 'void (void *, struct _xmlError *)') to
parameter of type '
xmlStructuredErrorFunc' (aka 'void (*)(void *, const struct _xmlError *)')
[-Wincompatible-function-pointer-types]
xmlSetStructuredErrorFunc((void *) xtCxt->xmlerrcxt,
xml_errorHandler);

 ^~~~
/usr/include/libxml2/libxml/xmlerror.h:898:29: note: passing argument to
parameter 'handler' here
 xmlStructuredErrorFunc handler);
^
xml.c:5003:55: error: incompatible function pointer types passing 'void
(void *, xmlErrorPtr)' (aka 'void (void *, struct _xmlError *)') to
parameter of type '
xmlStructuredErrorFunc' (aka 'void (*)(void *, const struct _xmlError *)')
[-Wincompatible-function-pointer-types]
xmlSetStructuredErrorFunc((void *) xtCxt->xmlerrcxt,
xml_errorHandler);

 ^~~~
/usr/include/libxml2/libxml/xmlerror.h:898:29: note: passing argument to
parameter 'handler' here
 xmlStructuredErrorFunc handler);
^
4 errors generated.
make[4]: *** [: xml.o] Error 1


Here is a quick workaround for it.

[1]
https://github.com/GNOME/libxml2/commit/61034116d0a3c8b295c6137956adc3ae55720711
[2]
https://github.com/GNOME/libxml2/commit/45470611b047db78106dcb2fdbd4164163c15ab7

Best Regards,
Xing
From bded3361f7f2ac7b4734d59de8ddb07875d9489d Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Sun, 3 Dec 2023 23:09:51 +0800
Subject: [PATCH v1] Make PostgreSQL work with newer version of libxml2.

There's a breaking change of API since v2.12.0 of libxml2. This patch
helps resolve it.

See: https://github.com/GNOME/libxml2/commit/61034116d0a3c8b295c6137956adc3ae55720711
---
 src/backend/utils/adt/xml.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index c401e7b821..8a391ca5fa 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -124,7 +124,11 @@ static xmlParserInputPtr xmlPgEntityLoader(const char *URL, const char *ID,
 		   xmlParserCtxtPtr ctxt);
 static void xml_errsave(Node *escontext, PgXmlErrorContext *errcxt,
 		int sqlcode, const char *msg);
+#if LIBXML_VERSION >= 21200
+static void xml_errorHandler(void *data, const xmlError *error);
+#else
 static void xml_errorHandler(void *data, xmlErrorPtr error);
+#endif
 static int	errdetail_for_xml_code(int code);
 static void chopStringInfoNewlines(StringInfo str);
 static void appendStringInfoLineSeparator(StringInfo str);
@@ -2023,8 +2027,11 @@ xml_errsave(Node *escontext, PgXmlErrorContext *errcxt,
 /*
  * Error handler for libxml errors and warnings
  */
-static void
-xml_errorHandler(void *data, xmlErrorPtr error)
+#if LIBXML_VERSION >= 21200
+static void xml_errorHandler(void *data, const xmlError *error)
+#else
+static void xml_errorHandler(void *data, xmlErrorPtr error)
+#endif
 {
 	PgXmlErrorContext *xmlerrcxt = (PgXmlErrorContext *) data;
 	x

Re: Don't pass NULL pointer to strcmp().

2023-11-02 Thread Xing Guo 
Hi,

Seems that Tom's patch cannot be applied to the current master branch.
I just re-generate the patch for others to play with.

On 11/2/23, Nathan Bossart  wrote:
> On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote:
>> Nathan Bossart  writes:
>>> What if we disallowed NULL string GUCs in v17?
>>
>> Well, we'd need to devise some other solution for hacks like the
>> one used by timezone_abbreviations (see comment in
>> check_timezone_abbreviations).  I think it's not worth the trouble,
>> especially seeing that 95% of guc.c is already set up for this.
>> The bugs are mostly in newer code like get_explain_guc_options,
>> and I think that's directly traceable to the lack of any comments
>> or docs about this behavior.
>
> Eh, yeah, it's probably not worth it if we find ourselves trading one set
> of hacks for another.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>


-- 
Best Regards,
Xing
From d620d3a2b44cef63024baca4ccdf55bf7724737f Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Thu, 2 Nov 2023 16:42:00 +0800
Subject: [PATCH v3] Consider treating NULL is a valid boot_val for string
 GUCs.

---
 src/backend/utils/misc/guc.c   | 28 
 src/include/utils/guc_tables.h | 10 ++
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..28056af19c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,9 @@ check_GUC_init(struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+	(conf->boot_val == NULL ||
+	 strcmp(*conf->variable, conf->boot_val) != 0))
 {
 	elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
 		 conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -4213,8 +4215,7 @@ SetConfigOption(const char *name, const char *value,
 /*
  * Fetch the current value of the option `name', as a string.
  *
- * If the option doesn't exist, return NULL if missing_ok is true (NOTE that
- * this cannot be distinguished from a string variable with a NULL value!),
+ * If the option doesn't exist, return NULL if missing_ok is true,
  * otherwise throw an ereport and don't return.
  *
  * If restrict_privileged is true, we also enforce that only superusers and
@@ -4257,7 +4258,8 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
 			return buffer;
 
 		case PGC_STRING:
-			return *((struct config_string *) record)->variable;
+			return *((struct config_string *) record)->variable ?
+*((struct config_string *) record)->variable : "";
 
 		case PGC_ENUM:
 			return config_enum_lookup_by_value((struct config_enum *) record,
@@ -4304,7 +4306,8 @@ GetConfigOptionResetString(const char *name)
 			return buffer;
 
 		case PGC_STRING:
-			return ((struct config_string *) record)->reset_val;
+			return ((struct config_string *) record)->reset_val ?
+((struct config_string *) record)->reset_val : "";
 
 		case PGC_ENUM:
 			return config_enum_lookup_by_value((struct config_enum *) record,
@@ -5255,7 +5258,14 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	if (lconf->boot_val == NULL &&
+		*lconf->variable == NULL)
+		modified = false;
+	else if (lconf->boot_val == NULL ||
+		*lconf->variable == NULL)
+		modified = true;
+	else
+		modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
@@ -5482,7 +5492,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-fprintf(fp, "%s", *conf->variable);
+if (*conf->variable)
+	fprintf(fp, "%s", *conf->variable);
 			}
 			break;
 
@@ -6142,7 +6153,8 @@ RestoreGUCState(void *gucstate)
 {
 	struct config_string *conf = (struct config_string *) gconf;
 
-	guc_free(*conf->variable);
+	if (*conf->variable)
+		guc_free(*conf->variable);
 	if (conf->reset_val && conf->reset_val != *conf->variable)
 		guc_free(conf->reset_val);
 	if (conf->reset_extra && conf->reset_extra != gconf->extra)
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1ec9575570..0c38255961 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -240,6 +240,16 @@ struct config_real
 	void	   *reset_extra;
 };

Re: Don't pass NULL pointer to strcmp().

2023-11-01 Thread Xing Guo 
Thank you Tom!

Your comment
"NULL doesn't have semantics that are visibly different from an empty
string" is exactly what I want to confirm :-)

On 11/2/23, Tom Lane  wrote:
> I wrote:
>> Hmm ... if we're doing it ourselves, I suppose we've got to consider
>> it supported :-(.  But I'm still wondering how many seldom-used
>> code paths didn't get the message.  An example here is that this
>> could lead to GetConfigOptionResetString returning NULL, which
>> I think is outside its admittedly-vague API spec.
>
> After digging around for a bit, I think part of the problem is a lack
> of a clearly defined spec for what should happen with NULL string GUCs.
> In the attached v3, I attempted to remedy that by adding a comment in
> guc_tables.h (which is maybe not the best place but I didn't see a
> better one).  That led me to a couple more changes beyond what you had.
>
> It's possible that some of these are unreachable --- for example,
> given that a NULL could only be the default value, I'm not sure that
> the fix in write_one_nondefault_variable is a live bug.  But we ought
> to code all this stuff defensively, and most of it already was
> NULL-safe.
>
>   regards, tom lane
>
>


-- 
Best Regards,
Xing

Re: Don't pass NULL pointer to strcmp().

2023-11-01 Thread Xing Guo 
Hi Tom,

There're extensions that set their boot_val to NULL. E.g., postgres_fdw (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/contrib/postgres_fdw/option.c#L582),
plperl (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L422C13-L422C13,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L444C12-L444C12,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L452C6-L452C6)
(Can we treat plperl as an extension?), pltcl (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L465C14-L465C14,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L472C12-L472C12
).

TBH, I don't know if NULL is a valid boot_val for string variables, I just
came across some extensions that use NULL as their boot_val. If the
boot_val can't be NULL in extensions, we should probably add some
assertions or comments about it?

Best Regards,
Xing








On Wed, Nov 1, 2023 at 11:30 PM Tom Lane  wrote:

> Xing Guo  writes:
> > Thanks for your comments. I have updated the patch accordingly.
>
> I'm leery of accepting this patch, as I see no reason that we
> should consider it valid for an extension to have a string GUC
> with a boot_val of NULL.
>
> I realize that we have a few core GUCs that are like that, but
> I'm pretty sure that every one of them has special-case code
> that initializes the GUC to something non-null a bit later on
> in startup.  I don't think there are any cases where a string
> GUC's persistent value will be null, and I don't like the
> idea of considering that to be an allowed case.  It would
> open the door to more crash situations, and it brings up the
> old question of how could a user tell NULL from empty string
> (via SHOW or current_setting() or whatever).  Besides, what's
> the benefit really?
>
> regards, tom lane
>

Re: Don't pass NULL pointer to strcmp().

2023-11-01 Thread Xing Guo 
Hi Aleksander and Junwang,

Thanks for your comments. I have updated the patch accordingly.

Best Regards,
Xing








On Wed, Nov 1, 2023 at 7:44 PM Aleksander Alekseev 
wrote:

> Hi,
>
> > > I found that there's a nullable pointer being passed to strcmp() and
> > > can make the server crash. It can be reproduced on the latest master
> > > branch by crafting an extension[1]. Patch for fixing it is attatched.
> > >
> > > [1] https://github.com/higuoxing/guc_crash/tree/pg
>
> Thanks for reporting. I can confirm that the issue reproduces on the
> `master` branch and the proposed patch fixes it.
>
> > Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
> > be unnecessary.
>
> Judging by the rest of the code we better keep it, at least for consistenc.
>
> I see one more place with a similar code in guc.c around line 1472.
> Although I don't have exact steps to trigger a crash I suggest adding
> a similar check there.
>
> --
> Best regards,
> Aleksander Alekseev
>
From bf0a9f848e69097a034692ed7b0be0ad81a5d991 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Wed, 1 Nov 2023 21:00:13 +0800
Subject: [PATCH v2] Don't use strcmp() with nullable pointers.

Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
 src/backend/utils/misc/guc.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..1d62523412 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,8 @@ check_GUC_init(struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+	(conf->boot_val == NULL || strcmp(*conf->variable, conf->boot_val) != 0))
 {
 	elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
 		 conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -5255,7 +5256,9 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
-- 
2.42.0

Don't pass NULL pointer to strcmp().

2023-11-01 Thread Xing Guo 
Hi hackers,

I found that there's a nullable pointer being passed to strcmp() and
can make the server crash. It can be reproduced on the latest master
branch by crafting an extension[1]. Patch for fixing it is attatched.

[1] https://github.com/higuoxing/guc_crash/tree/pg

-- 
Best Regards,
Xing
From dcd7a49190f0e19ba0a1e697cac45724450f6365 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Wed, 1 Nov 2023 16:41:49 +0800
Subject: [PATCH] Don't use strcmp() with nullable pointers.

Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
 src/backend/utils/misc/guc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..b277c48925 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5255,7 +5255,9 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
-- 
2.42.0

Remove obsolete check for TTSOpsVirtual from slot_compile_deform

2023-10-30 Thread Xing Guo 
Hi hackers,

While exploring the JIT support for tuple deforming process, I noticed that
one check for TTSOpsVirtual in slot_compile_deform is obsolete. Since
virtual tuples never need deforming and there's an assertion in
llvm_compile_expr[1]. I simply replace it with an assertion in
slot_compile_deform. Patch is attached.

[1]
https://github.com/postgres/postgres/blob/0c60e8ba80e03491b028204a19a9dca6d216df91/src/backend/jit/llvm/llvmjit_expr.c#L322

Best Regards,
Xing
From 6db602bd7d0964b60134c4b20b4107a1e8717b78 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Mon, 30 Oct 2023 22:34:10 +0800
Subject: [PATCH v1] Remove obsolete check for TTSOpsVirtual from
 slot_compile_deform.

Since virtual tuples never need deforming and there's no
EEOP_*_FETCHSOME step generated for it. We don't need to check it in
slot_compile_deform. Probably we should replace it with an assertion.
---
 src/backend/jit/llvm/llvmjit_deform.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
index b7e88e7ca9..e652cda77f 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -90,9 +90,8 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 
 	int			attnum;
 
-	/* virtual tuples never need deforming, so don't generate code */
-	if (ops == )
-		return NULL;
+	/* virtual tuples never need deforming */
+	Assert(ops != );
 
 	/* decline to JIT for slot types we don't know to handle */
 	if (ops !=  && ops !=  &&
-- 
2.42.0

Re: Guiding principle for dropping LLVM versions?

2023-10-21 Thread Xing Guo 
Hi,

Can we also check if the clang's version is compatible with llvm's version
in llvm.m4? I have multiple llvm toolchains installed on my system and I
have to specify the $CLANG and $LLVM_CONFIG variables each time I build the
server against a toolchain that is not present in $PATH. If one of the
variables is missing, the build system will pick up a default one whose
version might not be compatible with the other. E.g., If we use clang-16
and llvm-config-15, there will be issues when creating indexes for bitcodes
at the end of installation.

There will be errors look like

```
LLVM ERROR: ThinLTO cannot create input file: Unknown attribute kind (86)
(Producer: 'LLVM16.0.6' Reader: 'LLVM 15.0.7')
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/
and include the crash backtrace.
Stack dump:
0.  Program arguments: /usr/lib/llvm15/bin/llvm-lto -thinlto
-thinlto-action=thinlink -o postgres.index.bc postgres/access/brin/brin.bc
postgres/access/brin/brin_bloom.bc postgres/acces
s/brin/brin_inclusion.bc postgres/access/brin/brin_minmax.bc
postgres/access/brin/brin_minmax_multi.bc
postgres/access/brin/brin_pageops.bc postgres/access/brin/brin_revmap.bc
postgres/acce
ss/brin/brin_tuple.bc postgres/access/brin/brin_validate.bc
postgres/access/brin/brin_xlog.bc postgres/access/common/attmap.bc
postgres/access/common/bufmask.bc postgres/access/common/detoa
st.bc postgres/access/common/heaptuple.bc
postgres/access/common/indextuple.bc postgres/access/common/printsimple.bc
postgres/access/common/printtup.bc postgres/access/common/relation.bc po
stgres/access/common/reloptions.bc postgres/access/common/scankey.bc
postgres/access/common/session.bc postgres/access/common/syncscan.bc
postgres/access/common/toast_compression.bc postgre
s/access/common/toast_internals.bc postgres/access/common/tupconvert.bc
postgres/access/common/tupdesc.bc postgres/access/gin/ginarrayproc.bc
postgres/access/gin/ginbtree.bc postgres/access
/gin/ginbulk.bc postgres/access/gin/gindatapage.bc
postgres/access/gin/ginentrypage.bc postgres/access/gin/ginfast.bc
postgres/access/gin/ginget.bc postgres/access/gin/gininsert.bc postgres
/access/gin/ginlogic.bc postgres/access/gin/ginpostinglist.bc
postgres/access/gin/ginscan.bc postgres/access/gin/ginutil.bc
postgres/access/gin/ginvacuum.bc postgres/access/gin/ginvalidate.
bc postgres/access/gin/ginxlog.bc postgres/access/gist/gist.bc
postgres/access/gist/gistbuild.bc postgres/access/gist/gistbuildbuffers.bc
postgres/access/gist/gistget.bc postgres/access/gis
t/gistproc.bc postgres/access/gist/gistscan.bc
postgres/access/gist/gistsplit.bc postgres/access/gist/gistutil.bc
postgres/access/gist/gistvacuum.bc postgres/access/gist/gistvalidate.bc pos
tgres/access/gist/gistxlog.bc postgres/access/hash/hash.bc
postgres/access/hash/hash_xlog.bc postgres/access/hash/hashfunc.bc
postgres/access/hash/hashinsert.bc postgres/access/hash/hashovf
l.bc postgres/access/hash/hashpage.bc postgres/access/hash/hashsearch.bc
postgres/access/hash/hashsort.bc postgres/access/hash/hashutil.bc
postgres/access/hash/hashvalidate.bc postgres/acce
ss/heap/heapam.bc postgres/access/heap/heapam_handler.bc
postgres/access/heap/heapam_visibility.bc postgres/access/heap/heaptoast.bc
postgres/access/heap/hio.bc postgres/access/heap/prunehe
```

If we can check the llvm-config versions and clang versions at the
configuration phase we can detect the problem earlier.

Best Regards,
Xing








On Sun, Oct 22, 2023 at 10:07 AM Thomas Munro 
wrote:

> Rebased.  I also noticed this woefully out of date line:
>
> -  PGAC_PATH_PROGS(LLVM_CONFIG, llvm-config llvm-config-7
> llvm-config-6.0 llvm-config-5.0 llvm-config-4.0 llvm-config-3.9)
> +  PGAC_PATH_PROGS(LLVM_CONFIG, llvm-config llvm-config-17
> llvm-config-16 llvm-config-15 llvm-config-14)
>

Re: [RFC] Clang plugin for catching suspicious typedef casting

2023-08-14 Thread Xing Guo 
Hi,

On 8/10/23, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote:
>> On 03.08.23 18:56, Dmitry Dolgov wrote:
>> > I would like to get your opinion, folks. Does it sound interesting
>> > enough for the community, is it worth it to pursue the idea?
>>
>> I think it's interesting, and doesn't look too invasive.

+1.

>> Maybe we can come up with three or four interesting use cases and try to
>> implement them.  BlockNumber vs. Buffer type checking is obviously a bit
>> marginal to get anyone super-excited, but it's a reasonable demo.

Several months ago, I implemented a clang plugin[1] for checking
suspicious return/goto/break/continue in PG_TRY/PG_CATCH blocks and it
found unsafe codes in Postgres[2]. It's implemented using clang's AST
matcher API.

I would like to contribute it well if this RFC can be accepted.

[1] 
https://github.com/higuoxing/clang-plugins/blob/main/lib/ReturnInPgTryBlockChecker.cpp
[2] 
https://www.postgresql.org/message-id/flat/CACpMh%2BCMsGMRKFzFMm3bYTzQmMU5nfEEoEDU2apJcc4hid36AQ%40mail.gmail.com

-- 
Best Regards,
Xing

Re: PL/Python: Fix return in the middle of PG_TRY() block.

2023-05-04 Thread Xing Guo 
Sorry for not responding to this thread for a long time and a huge thank
you for pushing it forward!

Best Regards,
Xing








On Fri, May 5, 2023 at 7:42 AM Nathan Bossart 
wrote:

> On Thu, May 04, 2023 at 08:39:03AM -0400, Tom Lane wrote:
> > Hmm, I'm not sure why PLy_trigger_build_args's pltargs needs to
> > gain a "volatile" here?  LGTM otherwise.
>
> I removed that new "volatile" marker before committing.  I was trying to
> future-proof a bit and follow elog.h's recommendation to the letter, but
> following your mental model upthread, it doesn't seem to be strictly
> necessary, and we'd need to set pltargs to NULL after decrementing its
> reference count in the PG_TRY section for such future-proofing to be
> effective, anyway.
>
> Thank you for reviewing!
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>

Re: PL/Python: Fix return in the middle of PG_TRY() block.

2023-01-18 Thread Xing Guo 
On Mon, Jan 16, 2023 at 11:29 PM Tom Lane  wrote:

> Xing Guo  writes:
> > Are there any unsafe codes in pltcl.c? The return statement is in the
> > PG_CATCH() block, I think the exception stack has been recovered in
> > PG_CATCH block so the return statement in PG_CATCH block should be ok?
>
> Yes, the stack has already been unwound at the start of a PG_CATCH
> (or PG_FINALLY) block, so there is no reason to avoid returning
> out of those.
>
> In principle you could also mess things up with a "continue", "break",
> or "goto" leading out of PG_TRY.  That's probably far less likely
> than "return", but I wonder whether Andres' compiler hack will
> catch that.
>
> regards, tom lane
>

Thank you Tom,

Based on your comments, I've refactored my clang checker[1], now it can
warn about the following patterns:
1. return statement in PG_TRY(). We've catched all of them in this thread.
2. continue statement in PG_TRY() *unless* it's in for/while/do-while
statements.
3. break statement in PG_TRY() *unless* it's in for/while/do-while/switch
statements.
4. goto statement in PG_TRY() *unless* the label it points to is in the
same PG_TRY block.

Good news is that, there's no patterns (2, 3, 4) in Postgres source tree
and we've catched all of the return statements in the PG_TRY block in this
thread.

[1]
https://github.com/higuoxing/clang-plugins/blob/main/lib/ReturnInPgTryBlockChecker.cpp

Best Regards,
Xing

Re: PL/Python: Fix return in the middle of PG_TRY() block.

2023-01-16 Thread Xing Guo 
Hi,

I revised my patch, added the missing one that Nathan mentioned.

Are there any unsafe codes in pltcl.c? The return statement is in the
PG_CATCH() block, I think the exception stack has been recovered in
PG_CATCH block so the return statement in PG_CATCH block should be ok?

```
PG_TRY();
{
UTF_BEGIN;
ereport(level,
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg("%s", UTF_U2E(Tcl_GetString(objv[2]);
UTF_END;
}
PG_CATCH();
{
ErrorData  *edata;

/* Must reset elog.c's state */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();

/* Pass the error data to Tcl */
pltcl_construct_errorCode(interp, edata);
UTF_BEGIN;
Tcl_SetObjResult(interp, Tcl_NewStringObj(UTF_E2U(edata->message), -1));
UTF_END;
FreeErrorData(edata);

return TCL_ERROR;
}
PG_END_TRY();
```

Best Regards,
Xing








On Sat, Jan 14, 2023 at 2:03 AM Nathan Bossart 
wrote:

> On Thu, Jan 12, 2023 at 09:49:00PM -0800, Andres Freund wrote:
> > On 2023-01-12 10:44:33 -0800, Nathan Bossart wrote:
> >> There's another "return" later on in this PG_TRY block.  I wonder if
> it's
> >> possible to detect this sort of thing at compile time.
> >
> > Clang provides some annotations that allow to detect this kind of thing.
> I
> > hacked up a test for this, and it finds quite a bit of prolematic
> > code.
>
> Nice!
>
> > plpython is, uh, not being good? But also in plperl, pltcl.
>
> Yikes.
>
> > ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1:
> warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path
> through here [-Wthread-safety-analysis]
> > }
> > ^
> > ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note:
> no_returns_in_pg_try acquired here
> > PG_CATCH();
> > ^
> > ../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7:
> note: expanded from macro 'PG_CATCH'
> > no_returns_start(no_returns_handle##__VA_ARGS__)
> > ^
> >
> > Not perfect digestible, but also not too bad. I pushed the
> > no_returns_start()/no_returns_stop() calls into all the PG_TRY related
> macros,
> > because that causes the warning to point to block that the problem is
> > in. E.g. above the first warning points to PG_TRY, the second to
> > PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.
>
> This seems roughly as digestible as the pg_prevent_errno_in_scope stuff.
> However, on my macOS machine with clang 14.0.0, the messages say "mutex"
> instead of "no_returns_in_pg_try," which is unfortunate since that's the
> part that would clue me into what the problem is.  I suppose it'd be easy
> enough to figure out after a grep or two, though.
>
> > Clearly this would need a bunch more work, but it seems promising? I
> think
> > there'd be other uses than this.
>
> +1
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..7181b5e898 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -414,12 +414,12 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 	PyObject   *volatile args = NULL;
 	int			i;
 
+	args = PyList_New(proc->nargs);
+	if (!args)
+		return NULL;
+
 	PG_TRY();
 	{
-		args = PyList_New(proc->nargs);
-		if (!args)
-			return NULL;
-
 		for (i = 0; i < proc->nargs; i++)
 		{
 			PLyDatumToOb *arginfo = >args[i];
@@ -684,18 +684,28 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			   *pltrelid,
 			   *plttablename,
 			   *plttableschema;
-	PyObject   *pltargs,
+	PyObject   *pltargs = NULL,
 			   *pytnew,
 			   *pytold;
 	PyObject   *volatile pltdata = NULL;
 	char	   *stroid;
 
-	PG_TRY();
+	pltdata = PyDict_New();
+	if (!pltdata)
+		return NULL;
+
+	if (tdata->tg_trigger->tgnargs)
 	{
-		pltdata = PyDict_New();
-		if (!pltdata)
+		pltargs = PyList_New(tdata->tg_trigger->tgnargs);
+		if (!pltargs)
+		{
+			Py_DECREF(pltdata);
 			return NULL;
+		}
+	}
 
+	PG_TRY();
+	{
 		pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
 		PyDict_SetItemString(pltdata, "name", pltname);
 		Py_DECREF(pltname);
@@ -835,12 +845,6 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			int			i;
 			PyObject   *pltarg;
 
-			pltargs = PyList_New(tdata->tg_trigger->tgnargs);
-			if (!pltargs)
-			{
-Py_DECREF(pltdata);
-return NULL;
-			}
 			for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
 			{
 pltarg = PLyUnicode_FromString(tdata->tg_trigger->tgargs[i]);

Re: PL/Python: Fix return in the middle of PG_TRY() block.

2023-01-13 Thread Xing Guo 
Hi Nathan.

On 1/13/23, Nathan Bossart  wrote:
> On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
>> I was running static analyser against PostgreSQL and found there're 2
>> return statements in PL/Python module which is not safe. Patch is
>> attached.
>
> Is the problem that PG_exception_stack and error_context_stack aren't
> properly reset?

Yes, it is.

>
>> @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo,
>> PLyProcedure *proc, HeapTuple *r
>>  PyObject   *volatile pltdata = NULL;
>>  char   *stroid;
>>
>> +pltdata = PyDict_New();
>> +if (!pltdata)
>> +return NULL;
>> +
>>  PG_TRY();
>>  {
>> -pltdata = PyDict_New();
>> -if (!pltdata)
>> -return NULL;
>> -
>>  pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
>>  PyDict_SetItemString(pltdata, "name", pltname);
>>  Py_DECREF(pltname);
>
> There's another "return" later on in this PG_TRY block.  I wonder if it's
> possible to detect this sort of thing at compile time.

Thanks for pointing it out! I missed some return statements. Because
my checker is treating 'return statement in PG_TRY() block' as errors.
It stops compiling when it finds the code pattern and I forget to
double check it.

My checker is based on AST matcher and it's possible to turn it into a
clang-tidy plugin or something similar. I want to make it easy to
integrate with scan-build, so I implement it as a static analyzer
plugin :)

If anyone is interested in the checker itself, the source code can be
found here[1]:

> Note also:
> src/pl/tcl/pltcl.c- *   PG_CATCH();
> src/pl/tcl/pltcl.c- *   {
> src/pl/tcl/pltcl.c- *   pltcl_subtrans_abort(interp, oldcontext,
> oldowner);
> src/pl/tcl/pltcl.c- *   return TCL_ERROR;
> src/pl/tcl/pltcl.c- *   }
>
> This is documented once, repeated twice:
> src/pl/plpython/plpy_spi.c- *   PG_CATCH();
> src/pl/plpython/plpy_spi.c- *   {
> src/pl/plpython/plpy_spi.c- *   
> src/pl/plpython/plpy_spi.c- *   PLy_spi_subtransaction_abort(oldcontext,
> oldowner);
> src/pl/plpython/plpy_spi.c- *   return NULL;
> src/pl/plpython/plpy_spi.c- *   }
>
> I don't quite get why this would be a sane thing to do here when
> aborting a subtransaction in pl/python, but my experience with this
> code is limited.

Hi Michael,

I'll try to understand what's going on in your pasted codes. Thanks
for pointing it out!

> Example complaints:
>
> [776/1239 42  62%] Compiling C object
> src/pl/plpython/plpython3.so.p/plpy_exec.c.o
> ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:472:1:
> warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path
> through here [-Wthread-safety-analysis]
> }
> ^
> ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:417:2:
> note: no_returns_in_pg_try acquired here
> PG_TRY();
> ^
> ../../../../home/andres/src/postgresql/src/include/utils/elog.h:424:7: note:
> expanded from macro 'PG_TRY'
> no_returns_start(no_returns_handle##__VA_ARGS__)
> ^
> ...
> [785/1239 42  63%] Compiling C object src/pl/tcl/pltcl.so.p/pltcl.c.o
> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning:
> no_returns_in_pg_try 'no_returns_handle' is not held on every path through
> here [-Wthread-safety-analysis]
> }
> ^
> ../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note:
> no_returns_in_pg_try acquired here
> PG_CATCH();
> ^
> ../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note:
> expanded from macro 'PG_CATCH'
> no_returns_start(no_returns_handle##__VA_ARGS__)
> ^

Hi Andres,

Your patch looks interesting and useful. I will play with it in the
next following days. I'm burried with other works recently, so my
reply may delay.

> Not perfect digestible, but also not too bad. I pushed the
> no_returns_start()/no_returns_stop() calls into all the PG_TRY related
> macros,
> because that causes the warning to point to block that the problem is
> in. E.g. above the first warning points to PG_TRY, the second to
> PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.
>
>
> Clearly this would need a bunch more work, but it seems promising? I think
> there'd be other uses than this.
>
>
> I briefly tried to use it for spinlocks. Mostly works and detects things
> like
> returning with a spinlock held. But it does not like dynahash's habit of
> conditionally acquiring and releasing spinlocks.
>
> Gree

PL/Python: Fix return in the middle of PG_TRY() block.

2023-01-12 Thread Xing Guo 
Hi hackers,

I was running static analyser against PostgreSQL and found there're 2
return statements in PL/Python module which is not safe. Patch is
attached.

-- 
Best Regards,
Xing
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..c0e4a81283 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -414,12 +414,12 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 	PyObject   *volatile args = NULL;
 	int			i;
 
+	args = PyList_New(proc->nargs);
+	if (!args)
+		return NULL;
+
 	PG_TRY();
 	{
-		args = PyList_New(proc->nargs);
-		if (!args)
-			return NULL;
-
 		for (i = 0; i < proc->nargs; i++)
 		{
 			PLyDatumToOb *arginfo = >args[i];
@@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 	PyObject   *volatile pltdata = NULL;
 	char	   *stroid;
 
+	pltdata = PyDict_New();
+	if (!pltdata)
+		return NULL;
+
 	PG_TRY();
 	{
-		pltdata = PyDict_New();
-		if (!pltdata)
-			return NULL;
-
 		pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
 		PyDict_SetItemString(pltdata, "name", pltname);
 		Py_DECREF(pltname);

Re: [PATCH] Simple code cleanup in tuplesort.c.

2023-01-09 Thread Xing Guo 
On 1/9/23, John Naylor  wrote:
> On Thu, Jan 5, 2023 at 8:18 AM John Naylor 
> wrote:
>>
>> The label TSS_BUILDRUNS has a similar style and also the following
> comment, so I will push this patch with a similar comment added unless
> someone wants to make a case for doing otherwise.
>>
>> * Note that mergeruns sets the correct state->status.
>
> This has been pushed, thanks. Note that both patches in this thread have
> the same name. Adding a version number to the name is a good way to
> distinguish them.

Thank you John. This is my first patch, I'll keep it in mind that
adding a version number next time I sending the patch.

> --
> John Naylor
> EDB: http://www.enterprisedb.com
>


-- 
Best Regards,
Xing

pg_regress/pg_isolation_regress: Fix possible nullptr dereference.

2022-11-30 Thread Xing Guo 
Hi hackers,

While playing with pg_regress and pg_isolation_regress, I noticed that
there's a potential nullptr deference in both of them.

How to reproduce:

Specify the `--dbname=` option without providing any database name.

//pg_regress --dbname=   foo
//pg_isolation_regress --dbname=   foo

Patch is attached.

-- 
Best Regards,
Xing
diff --git a/src/test/isolation/isolation_main.c 
b/src/test/isolation/isolation_main.c
index 31a0e6b709..a88e9da255 100644
--- a/src/test/isolation/isolation_main.c
+++ b/src/test/isolation/isolation_main.c
@@ -89,7 +89,7 @@ isolation_start_test(const char *testname,
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
   "\"%s\" \"dbname=%s\" < \"%s\" > 
\"%s\" 2>&1",
   isolation_exec,
-  dblist->str,
+  dblist ? dblist->str : "",
   infile,
   outfile);
if (offset >= sizeof(psql_cmd))
diff --git a/src/test/regress/pg_regress_main.c 
b/src/test/regress/pg_regress_main.c
index a4b354c9e6..4e089b0f83 100644
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -81,7 +81,7 @@ psql_start_test(const char *testname,
   "\"%s%spsql\" -X -a -q -d \"%s\" %s 
< \"%s\" > \"%s\" 2>&1",
   bindir ? bindir : "",
   bindir ? "/" : "",
-  dblist->str,
+  dblist ? dblist->str : "",
   "-v HIDE_TABLEAM=on -v 
HIDE_TOAST_COMPRESSION=on",
   infile,
   outfile);

Re: [PATCH] Simple code cleanup in tuplesort.c.

2022-09-30 Thread Xing Guo 
Hi Richard,

Sorry for not responding for a long time, I missed the previous email
by accident :-)

I attached a newer patch based on your suggestions and created an
entry in cf manager.
https://commitfest.postgresql.org/40/3924/

Best Regards,
Xing Guo



On 9/16/22, Richard Guo  wrote:
> On Wed, Jul 27, 2022 at 5:10 PM Xing Guo  wrote:
>
>> The bounded heap sorting status flag is set twice in sort_bounded_heap()
>> and tuplesort_performsort(). This patch helps remove one of them.
>>
>
> Revisiting this patch I think maybe it's better to remove the setting of
> Tuplesort status from tuplesort_performsort() for the TSS_BOUNDED case.
> Thus we keep the heap manipulation routines, make_bounded_heap and
> sort_bounded_heap, consistent in that they update their status
> accordingly inside the function.
>
> Also, would you please add it to the CF to not lose track of it?
>
> Thanks
> Richard
>


-- 
Best Regards,
Xing
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 416f02ba3c..ef85e58aeb 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1445,7 +1445,6 @@ tuplesort_performsort(Tuplesortstate *state)
 			state->eof_reached = false;
 			state->markpos_offset = 0;
 			state->markpos_eof = false;
-			state->status = TSS_SORTEDINMEM;
 			break;
 
 		case TSS_BUILDRUNS:

[PATCH] Simple code cleanup in tuplesort.c.

2022-07-27 Thread Xing Guo 
Hi hackers,

The bounded heap sorting status flag is set twice in sort_bounded_heap()
and tuplesort_performsort(). This patch helps remove one of them.

Best Regards,
Xing
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index d90a1aebdf..84dc0d07f9 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2709,7 +2709,6 @@ sort_bounded_heap(Tuplesortstate *state)
 	 */
 	reversedirection(state);
 
-	state->status = TSS_SORTEDINMEM;
 	state->boundUsed = true;
 }

Re: try_relation_open and relation_open behave different.

2021-10-18 Thread Xing GUO 
On Mon, Oct 18, 2021 at 2:45 PM Michael Paquier  wrote:

> On Mon, Oct 18, 2021 at 01:56:07PM +0800, Xing GUO wrote:
> > My question is, is it a deliberate design that makes try_relation_open
> and
> > relation_open different? Shall we mention it in the comment of
> > try_relation_open OR adding the checker to relation_open?
>
> I am not sure what you mean here, both functions are include comments
> to explain their differences, so..
>

The comments in try_relation_open says:

```
/* 
 * try_relation_open - open any relation by relation OID
 *
 * Same as relation_open, except return NULL instead of failing
 * if the relation does not exist.
 * 
 */
```

However, I can open an "uncommitted" relation using relation_open() and
cannot open it using try_relation_open().
Since Postgres doesn't write the "uncommitted" relation descriptor to
SysCache and try_relation_open() checks if the
relation exists in SysCache while relation_open() doesn't check it.


> --
> Michael
>

try_relation_open and relation_open behave different.

2021-10-17 Thread Xing GUO 
Hi hackers,

I'm writing an extension that employs `object_access_hook`. I want to
monitor the table creation event and record the mapping between `reloid`
and `relfilenode` during a transaction. Here's my code snippet,

```
static void
my_object_access_hook(ObjectAccessType access,
  Oid classId,
  Oid objectId,
  int subId, void *arg)
{
do_some_checks(access, classId, ...);
// open the relation using relation_open
rel = relation_open(objectId, AccessShareLock);

// record the reloid and relfilenode.
record(objectId, rel->rd_node);
relation_close(rel, AccessShareLock);
}
```

However, when I replace the relation_open with try_relation_open, the
relation cannot be opened. I've checked the source code, it looks that
try_relation_open has an additional checker which causes the relation_open
and try_relation_open behavior different:

```
Relation
try_relation_open(Oid relationId, LOCKMODE lockmode)
{
...
/*
 * Now that we have the lock, probe to see if the relation really exists
 * or not.
 */
if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relationId)))
{
/* Release useless lock */
if (lockmode != NoLock)
   UnlockRelationOid(relationId, lockmode);

return NULL;
}
...
}
```

See:
https://github.com/postgres/postgres/blob/c30f54ad732ca5c8762bb68bbe0f51de9137dd72/src/backend/access/common/relation.c#L47

My question is, is it a deliberate design that makes try_relation_open and
relation_open different? Shall we mention it in the comment of
try_relation_open OR adding the checker to relation_open?

Best Regards,
Xing

Re: `make check` doesn't pass on MacOS Catalina

2021-04-20 Thread Xing GUO 
On 4/21/21, Tom Lane  wrote:
> Xing GUO  writes:
>> Thank you very much. I'm facing the same problem yesterday. May I
>> suggest that document it in the installation guide on MacOS platform?
>
> It is documented --- see last para under
>
> https://www.postgresql.org/docs/current/installation-platform-notes.html#INSTALLATION-NOTES-MACOS

Thank you! Sorry for my carelessness...

>
>   regards, tom lane
>


-- 
Best Regards,
Xing

Re: `make check` doesn't pass on MacOS Catalina

2021-04-20 Thread Xing GUO 
Hi hackers,

Thank you very much. I'm facing the same problem yesterday. May I
suggest that document it in the installation guide on MacOS platform?

On 4/21/21, Andrew Dunstan  wrote:
>
> On 4/20/21 11:02 AM, Tom Lane wrote:
>> Aleksander Alekseev  writes:
>>> While trying to build PostgreSQL from source (master branch, 95c3a195) on
>>> a
>>> MacBook I discovered that `make check` fails:
>> This is the usual symptom of not having disabled SIP :-(.
>>
>> If you don't want to do that, do "make install" before "make check".
>>
>>  
>
>
>
>
> FYI the buildfarm client has a '--delay-check' option that does exactly
> this. It's useful on Alpine Linux as well as MacOS
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>
>
>


-- 
Best Regards,
Xing