Re: Add assertion for failed alloc to palloc0() and palloc_extended()

[Michael Paquier]

On Sat, Mar 01, 2025 at 01:05:43AM +0100, Andreas Karlsson wrote:
> I noticed that we have Assert(ret != NULL) in palloc() but not in palloc0()
> so for consistency I decided to add it. I also added an assertion that the
> MCXT_ALLOC_NO_OOM flag is set if alloc() returns
> NULL to palloc_extended().
> 
> I feel that this might be useful since while palloc() is much more common
> the OOM which causes alloc() to incorrectly return NULL could in theory
> happen in any of the three functions.

Hmm.  Good points.  All the MemoryContextMethods rely on
MemoryContextAllocationFailure() to handle the case of
MCXT_ALLOC_NO_OOM on failure (except alignedalloc which has no alloc 
routine).  Your two suggestions, one in palloc0() for the non-NULL
check, and the second one in palloc_extended() to make sure that we
have MCXT_ALLOC_NO_OOM set when the result is NULL, could catch
inconsistencies when implementing a new method.

In short, LGTM.  Will apply if there are no objections.
--
Michael


signature.asc
Description: PGP signature

Accidental assignment instead of compare in GetOperatorFromCompareType?

[Jacob Brazeal]

Hi all,

It looks like commit 630f9a43cece93cb4a5c243b30e34abce6a89514 omitted an
equals sign in an ereport() in GetOperatorFromCompareType, giving the line:

cmptype = COMPARE_EQ ? errmsg("could not identify an equality operator for
type %s", format_type_be(opcintype)) :

I think the impact is just that we'd never see the error messages for
the COMPARE_OVERLAP and COMPARE_CONTAINED_BY cases logged.

Regards,
Jacob


v01_fix_typos.patch
Description: Binary data

Re: [PATCH] Add regression tests of ecpg command notice (error / warning)

[Ryo Kanbayashi]

>On Mon, Mar 3, 2025 at 12:23 PM Fujii Masao  
>wrote:
> On 2025/03/01 19:45, Ryo Kanbayashi wrote:
> >> +program_help_ok('ecpg');
> >> +program_version_ok('ecpg');
> >> +program_options_handling_ok('ecpg');
> >> +command_fails(['ecpg'], 'ecpg without arguments fails');
> >>
> >> These checks seem unnecessary in 002 since they're already covered in 001.
> >
> > I reflected above.
>
> Thanks for updating the patch!
>
> I've made some minor fixes and cosmetic adjustments.
> The updated patch is attached.
>
> Unless there are any objections, I'll commit it.

Thanks for reviewing and adustments.
There is no objections :)

---
Great regards,
Ryo Kanbayashi
NTT Open Source Software Center

Re: Accidental assignment instead of compare in GetOperatorFromCompareType?

[Jacob Brazeal]

Sorry, I attached the wrong patch.

>


v1_cmp_assign.patch
Description: Binary data

Re: Accidental assignment instead of compare in GetOperatorFromCompareType?

[Michael Paquier]

On Sun, Mar 02, 2025 at 09:06:18PM -0800, Jacob Brazeal wrote:
> Sorry, I attached the wrong patch.
 errcode(ERRCODE_UNDEFINED_OBJECT),
-cmptype = COMPARE_EQ ? errmsg("could not identify an 
equality operator for type %s", format_type_be(opcintype)) :
+cmptype == COMPARE_EQ ? errmsg("could not identify an 
equality operator for type %s", format_type_be(opcintype)) :

Yep, you're right.  That's a typo coming from the recent commit
630f9a43cece.
--
Michael


signature.asc
Description: PGP signature

Re: doc: Mention clock synchronization recommendation for hot_standby_feedback

[Amit Kapila]

On Wed, Jan 8, 2025 at 6:20 PM Jakub Wartak
 wrote:
>
> On Wed, Dec 18, 2024 at 10:33 AM Amit Kapila  wrote:
>
> Hi Amit!
>
> > On Thu, Dec 5, 2024 at 3:14 PM Jakub Wartak
> >  wrote:
> > >
> > > One of our customers ran into a very odd case, where hot standby feedback 
> > > backend_xmin propagation stopped working due to major (hours/days) clock 
> > > time shifts on hypervisor-managed VMs. This happens (and is fully 
> > > reproducible) e.g. in scenarios where standby connects and its own VM is 
> > > having time from the future (relative to primary) and then that time goes 
> > > back to "normal". In such situation "sends hot_standby_feedback xmin" 
> > > timestamp messages are stopped being transferred, e.g.:
> > >
> > > 2024-12-05 02:02:35 UTC [6002]: db=,user=,app=,client= DEBUG:  sending 
> > > hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 
> > > catalog_xmin_epoch 0
> > > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending 
> > > write 6/E9015230 flush 6/E9015230 apply 6/E9015230
> > > 2024-12-05 02:02:45 UTC [6002]: db=,user=,app=,client= DEBUG:  sending 
> > > hot standby feedback xmin 1614031 epoch 0 catalog_xmin 0 
> > > catalog_xmin_epoch 0
> > > <-- clock readjustment and no further "sending hot standby feedback"
> > ...
> > >
> > > I can share reproduction steps if anyone is interested. This basically 
> > > happens due to usage of TimestampDifferenceExceeds() in 
> > > XLogWalRcvSendHSFeedback(), but I bet there are other similiar scenarios.
> > >
> >
> > We started to use a different mechanism in HEAD. See 
> > XLogWalRcvSendHSFeedback().
>
> Yes, you are correct somewhat because I was looking on REL13_STABLE,
> but I've taken a fresh quick look at 05a7be93558 and tested it too.
> Sadly, PG17 still maintains the same behavior of lack of proper
> backend_xmin propagation (it stops sending hot standby feedback once
> time on standby jumps forward). I believe this is the case because
> walreceiver schedules next wakeup in far far future (when the clock /
> now() is way ahead, see WalRcvComputeNextWakeup()), so when the clock
> is back to normal (resetted back -X hours/days), the next wakeup seems
> to be +X hours/days ahead.
>
> > > What I was kind of surprised about was the lack of recommendation for 
> > > having primary/standby to have clocks synced when using 
> > > hot_standby_feedback, but such a thing is mentioned for 
> > > recovery_min_apply_delay. So I would like to add at least one sentence to 
> > > hot_standby_feedback to warn about this too, patch attached.
> > >
> >
> > IIUC, this issue doesn't occur because the primary and standby clocks
> > are not synchronized. It happened because the clock on standby moved
> > backward.
>
> In PG17 it would be because the clock moved way forward too much on
> the standby. I don't know how it happened to that customer, but it was
> probably done somehow by the hypervisor in that scenario (so time
> wasn't slewed improperly by ntpd AFAIR, edge case, I know...)
>
> > This is quite unlike the 'recovery_min_apply_delay' where
> > non-synchronization of clocks between primary and standby can lead to
> > unexpected results. This is because we don't compare any time on the
> > primary with the time on standby. If this understanding is correct
> > then the wording proposed by your patch should be changed accordingly.
>
> .. if my understanding is correct, it is both depending on version :^)
>

AFAICS, it doesn't depend on the version. I checked the code of PG13,
and it uses a similar implementation. I am referring to the below code
in PG13:
if (!immed)
{
/*
* Send feedback at most once per wal_receiver_status_interval.
*/
if (!TimestampDifferenceExceeds(sendTime, now,
wal_receiver_status_interval * 1000))
return;
sendTime = now;
}

> I was thinking about backpatching docs (of what is the recommended
> policy here? to just update new-release docs?), so I'm proposing
> something more generic than earlier, but it takes Your point into
> account - would something like below be good enough?
>
> -   
> -Using this option requires the primary and standby(s) to have system
> -clocks synchronized, otherwise it may lead to prolonged risk of not
> -removing dead rows on primary for extended periods of time as the
> -feedback mechanism is based on timestamps exchanged between primary
> -and standby(s).
> -   
>
> +   
> +Using this option requires the primary and standby(s) to have system
> +clocks synchronized (without big time jumps), otherwise it may lead 
> to
> +prolonged risk of not removing dead rows on primary for
> extended periods
> +of time as the feedback mechanism implementation is timestamp based.
> +   
>

How about something like: "Note that if the clock on standby is moved
ahead or backward, the feedback message may not be sent at the
required interval. This can lead to prolonged risk of not removing
dead rows on primary for ex

Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

[Michael Paquier]

On Fri, Feb 28, 2025 at 12:40:13PM -0800, Jacob Champion wrote:
> v9 removes the first call, and moves the second (now only) call up and
> out of the if/else chain, just past client authentication. The SSL
> pre-auth tests have been removed.

I have put my eyes on 0001, and this version looks sensible here, just
tweaked a bit the comments after a closer lookup and adjusted a few
things, nothing huge..

/* Update app name to current GUC setting */
+   /* TODO: ask the list: maybe do this before setting STATE_UNDEFINED? */
if (application_name)
pgstat_report_appname(application_name);

This has always been set last and it's still the case in the patch, so
let's just remove that.
--
Michael
From 301c381261c08efdcf2b129b8150db98eb101a97 Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Fri, 3 May 2024 15:54:58 -0700
Subject: [PATCH v10] pgstat: report in earlier with STATE_STARTING

Split pgstat_bestart() into three phases for better observability:

1) pgstat_bestart_initial() reports a 'starting' state while waiting for
   backend initialization and client authentication to complete.  Since
   we hold a transaction open for a good amount of that, and some
   authentication methods call out to external systems, having an early
   pg_stat_activity entry helps DBAs debug when things go badly wrong.

2) pgstat_bestart_security() reports the SSL/GSS status of the
   connection.  Some backends don't call this at all; others call it
   after client authentication.

3) pgstat_bestart_final() reports the user and database IDs, takes the
   entry out of STATE_STARTING, and reports the application_name.
   TODO: should the order of those last two be swapped?
---
 src/include/utils/backend_status.h  |   5 +-
 src/backend/postmaster/auxprocess.c |   3 +-
 src/backend/utils/activity/backend_status.c | 215 +---
 src/backend/utils/adt/pgstatfuncs.c |   3 +
 src/backend/utils/init/postinit.c   |  40 +++-
 src/test/authentication/Makefile|   4 +
 src/test/authentication/meson.build |   4 +
 src/test/authentication/t/007_pre_auth.pl   |  81 
 doc/src/sgml/monitoring.sgml|   6 +
 9 files changed, 275 insertions(+), 86 deletions(-)
 create mode 100644 src/test/authentication/t/007_pre_auth.pl

diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index d3d4ff6c5c9a..1c9b4fe14d06 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -24,6 +24,7 @@
 typedef enum BackendState
 {
 	STATE_UNDEFINED,
+	STATE_STARTING,
 	STATE_IDLE,
 	STATE_RUNNING,
 	STATE_IDLEINTRANSACTION,
@@ -309,7 +310,9 @@ extern void BackendStatusShmemInit(void);
 
 /* Initialization functions */
 extern void pgstat_beinit(void);
-extern void pgstat_bestart(void);
+extern void pgstat_bestart_initial(void);
+extern void pgstat_bestart_security(void);
+extern void pgstat_bestart_final(void);
 
 extern void pgstat_clear_backend_activity_snapshot(void);
 
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index ff366ceb0fc7..4f6795f72650 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -78,7 +78,8 @@ AuxiliaryProcessMainCommon(void)
 
 	/* Initialize backend status information */
 	pgstat_beinit();
-	pgstat_bestart();
+	pgstat_bestart_initial();
+	pgstat_bestart_final();
 
 	/* register a before-shutdown callback for LWLock cleanup */
 	before_shmem_exit(ShutdownAuxiliaryProcess, 0);
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 5f68ef26adc6..1a4ca2b179ca 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -253,31 +253,23 @@ pgstat_beinit(void)
 	on_shmem_exit(pgstat_beshutdown_hook, 0);
 }
 
-
-/* --
- * pgstat_bestart() -
+/* -
+ * pgstat_bestart_initial() -
  *
- *	Initialize this backend's entry in the PgBackendStatus array.
- *	Called from InitPostgres.
+ * Initialize this backend's entry in the PgBackendStatus array.
+ * Called from InitPostgres() and AuxiliaryProcessMainCommon().
  *
- *	Apart from auxiliary processes, MyDatabaseId, session userid, and
- *	application_name must already be set (hence, this cannot be combined
- *	with pgstat_beinit).  Note also that we must be inside a transaction
- *	if this isn't an aux process, as we may need to do encoding conversion
- *	on some strings.
- *--
+ * Clears out a new pgstat entry, initializing it to suitable defaults and
+ * reporting STATE_STARTING.  Backends should continue filling in any
+ * transport security details as needed with pgstat_bestart_security(), and
+ * must finally exit STATE_STARTING by calling pgstat_bestart_final().
+ * -
  */
 void
-pgstat_bestart(void)
+pgstat_bestart_initial(void)
 {
 	volatile PgBackendStatus *vbeentry = MyBEEntry;
 	PgBackendStatus lbeentry;
-

Re: Make COPY format extendable: Extract COPY TO format implementations

[Tom Lane]

Junwang Zhao  writes:
> While review another thread (Emitting JSON to file using COPY TO),
> I found the recently committed patches on this thread pass the
> CopyFormatOptions struct directly rather a pointer of the struct
> as a function parameter of CopyToGetRoutine and CopyFromGetRoutine.

Coverity is unhappy about that too:

/srv/coverity/git/pgsql-git/postgresql/src/backend/commands/copyto.c: 177 in 
CopyToGetRoutine()
171 .CopyToOneRow = CopyToBinaryOneRow,
172 .CopyToEnd = CopyToBinaryEnd,
173 };
174 
175 /* Return a COPY TO routine for the given options */
176 static const CopyToRoutine *
>>> CID 1643911:  Performance inefficiencies  (PASS_BY_VALUE)
>>> Passing parameter opts of type "CopyFormatOptions" (size 184 bytes) by 
>>> value, which exceeds the low threshold of 128 bytes.
177 CopyToGetRoutine(CopyFormatOptions opts)
178 {
179 if (opts.csv_mode)
180 return &CopyToRoutineCSV;

(and likewise for CopyFromGetRoutine).  I realize that these
functions aren't called often enough for performance to be an
overriding concern, but it still seems like poor style.

> Then I took a quick look at the newly rebased patch set and
> found Sutou has already fixed this issue.

+1, except I'd suggest declaring the parameters as
"const CopyFormatOptions *opts".

regards, tom lane

Re: 64 bit numbers vs format strings

[Peter Eisentraut]

On 05.12.24 23:18, Thomas Munro wrote:

Having learned some things about gettext based on clues[1] from Peter
E, I decided to see what it would take to expunge all (long long) and
similar casts now that we're using the standard types with system
support.

The short version is tha given uint64 x:

 Old: errmsg("hello %llu", (unsigned long long) x)
 New: errmsg("hello %" PRIu64, x)


I have committed the subset of this patch for pg_checksums.c so that the 
translators and whoever else might be affected can try this out at small 
scale.  (I don't expect any particular problems.)  Then we can move on 
to the rest in a few weeks, I think.

Re: Logging which local address was connected to in log_line_prefix

[Jim Jones]

On 27.02.25 14:54, Greg Sabino Mullane wrote:
> Great question. I think "supposed to" is a bit of a stretch, but I
> presume it's the difference between a client connecting and using its
> connection information versus an already existing backend process,
> which is always going to be "local".
>
> Overall this makes sense, as that checkpoint example above is coming
> from the checkpointer background process at 3114981, not the backend
> process that happened to trigger it. And 3114981 has no way of knowing
> the details of the caller's connection.
>
In that case, it LGTM.

I revisited this patch and tested it with two different computers (for
client and server).

Initially, I was momentarily confused by the logged address format,
which varies depending on the client's format. However, I found that %h
behaves just like this, so I guess it is ok.


postgres=# SHOW log_line_prefix;
    log_line_prefix    
---
 %m [%p]:  L=%L, h=%h
(1 row)


2025-03-02 18:19:07.859 CET [2246150]:  L=192.168.178.27,
h=192.168.178.79 ERROR:  division by zero
2025-03-02 18:19:07.859 CET [2246150]:  L=192.168.178.27,
h=192.168.178.79 STATEMENT:  SELECT 1/0

2025-03-02 18:19:19.327 CET [2246291]:  L=2a02:...:7591, h=2a02:...:4a7
ERROR:  division by zero
2025-03-02 18:19:19.327 CET [2246291]:  L=2a02:...:7591, h=2a02:...:4a7
STATEMENT:  SELECT 1/0


Best, Jim

Re: Add Postgres module info

[Andrei Lepikhov]

On 17/2/2025 02:41, Alexander Korotkov wrote:

On Mon, Dec 16, 2024 at 7:02 AM Andrei Lepikhov  wrote:

On 12/13/24 10:17, Tom Lane wrote:

There's nothing stopping a field of the magic block from being
a "const char *" pointer to a string literal.

Ok, See v.2 in attachment.


Generally, the patch looks good to me.  I have couple of questions.

1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?

I haven't such intention. Just wanted to demonstrate how it might work.


2) Once we have module version information, it looks natural to
specify the required version for dependant objects, e.g. SQL-funcions
implemented in shared libraries.  For instance,
CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
Just to be clear. You want this stuff to let the core manage situations 
of stale binaries and throw an error like the following:

"No function matches the given name, argument types and module version"
Do I understand you correctly?
It may make sense, but I can't figure out a use case. Could you describe 
at least one example?


--
regards, Andrei Lepikhov

Re: 64 bit numbers vs format strings

[Tom Lane]

Thomas Munro  writes:
> I also added one more patch that I expect to be more contentious as it
> is a UX change.  Why do we display LSNs with a slash?

While there's surely little reason to do that anymore, I think the
blast radius of such a change will be vastly greater than is warranted
by aesthetics.  It's not only our code that will be affected --- I'm
pretty sure there is a great deal of replication tooling out there
that this will break.  Don't expect me to defend you from the
villagers with pitchforks.

regards, tom lane

Re: Make COPY format extendable: Extract COPY TO format implementations

[Sutou Kouhei]

Hi,

In <3191030.1740932...@sss.pgh.pa.us>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on 
Sun, 02 Mar 2025 11:27:20 -0500,
  Tom Lane  wrote:

>> While review another thread (Emitting JSON to file using COPY TO),
>> I found the recently committed patches on this thread pass the
>> CopyFormatOptions struct directly rather a pointer of the struct
>> as a function parameter of CopyToGetRoutine and CopyFromGetRoutine.
> 
> Coverity is unhappy about that too:
> 
> /srv/coverity/git/pgsql-git/postgresql/src/backend/commands/copyto.c: 177 in 
> CopyToGetRoutine()
> 171   .CopyToOneRow = CopyToBinaryOneRow,
> 172   .CopyToEnd = CopyToBinaryEnd,
> 173 };
> 174 
> 175 /* Return a COPY TO routine for the given options */
> 176 static const CopyToRoutine *
 CID 1643911:  Performance inefficiencies  (PASS_BY_VALUE)
 Passing parameter opts of type "CopyFormatOptions" (size 184 bytes) by 
 value, which exceeds the low threshold of 128 bytes.
> 177 CopyToGetRoutine(CopyFormatOptions opts)
> 178 {
> 179   if (opts.csv_mode)
> 180   return &CopyToRoutineCSV;
> 
> (and likewise for CopyFromGetRoutine).  I realize that these
> functions aren't called often enough for performance to be an
> overriding concern, but it still seems like poor style.
> 
>> Then I took a quick look at the newly rebased patch set and
>> found Sutou has already fixed this issue.
> 
> +1, except I'd suggest declaring the parameters as
> "const CopyFormatOptions *opts".

Thanks for pointing out this (and sorry for missing this in
our reviews...)!

How about the attached patch?

I'll rebase the v35 patch set after this is fixed.


Thanks,
-- 
kou
>From f21b48c7dd0b141c561e9c8a2c9f1d0e28aabfae Mon Sep 17 00:00:00 2001
From: Sutou Kouhei 
Date: Mon, 3 Mar 2025 09:13:37 +0900
Subject: [PATCH] Use const pointer for CopyFormatOptions for
 Copy{To,From}GetRoutine()

We don't need to copy CopyFormatOptions here.

Reported-by: Junwang Zhao 
Discussion: https://postgr.es/m/CAEG8a3L6YCpPksTQMzjD_CvwDEhW3D_t=5md9BvvdOs5k+TA=q...@mail.gmail.com
---
 src/backend/commands/copyfrom.c | 8 
 src/backend/commands/copyto.c   | 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 198cee2bc48..bcf66f0adf8 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -153,11 +153,11 @@ static const CopyFromRoutine CopyFromRoutineBinary = {
 
 /* Return a COPY FROM routine for the given options */
 static const CopyFromRoutine *
-CopyFromGetRoutine(CopyFormatOptions opts)
+CopyFromGetRoutine(const CopyFormatOptions *opts)
 {
-	if (opts.csv_mode)
+	if (opts->csv_mode)
 		return &CopyFromRoutineCSV;
-	else if (opts.binary)
+	else if (opts->binary)
 		return &CopyFromRoutineBinary;
 
 	/* default is text */
@@ -1574,7 +1574,7 @@ BeginCopyFrom(ParseState *pstate,
 	ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
 
 	/* Set the format routine */
-	cstate->routine = CopyFromGetRoutine(cstate->opts);
+	cstate->routine = CopyFromGetRoutine(&cstate->opts);
 
 	/* Process the target relation */
 	cstate->rel = rel;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 721d29f8e53..84a3f3879a8 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -174,11 +174,11 @@ static const CopyToRoutine CopyToRoutineBinary = {
 
 /* Return a COPY TO routine for the given options */
 static const CopyToRoutine *
-CopyToGetRoutine(CopyFormatOptions opts)
+CopyToGetRoutine(const CopyFormatOptions *opts)
 {
-	if (opts.csv_mode)
+	if (opts->csv_mode)
 		return &CopyToRoutineCSV;
-	else if (opts.binary)
+	else if (opts->binary)
 		return &CopyToRoutineBinary;
 
 	/* default is text */
@@ -700,7 +700,7 @@ BeginCopyTo(ParseState *pstate,
 	ProcessCopyOptions(pstate, &cstate->opts, false /* is_from */ , options);
 
 	/* Set format routine */
-	cstate->routine = CopyToGetRoutine(cstate->opts);
+	cstate->routine = CopyToGetRoutine(&cstate->opts);
 
 	/* Process the source/target relation or query */
 	if (rel)
-- 
2.47.2

Fixing various typos in comments and docs

[Jacob Brazeal]

This patch fixes various typos I've found, most of them from recent
commits. I think none should be controversial except perhaps

--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -801,7 +801,7 @@ psql --username=postgres --file=script.sql postgres
 

 
- Because not all statistics are not transferred by
+ Because not all statistics are transferred by
  pg_upgrade, you will be instructed to run a
command to
  regenerate that information at the end of the upgrade.  You might
need to
  set connection parameters to match your new cluster.

Separately from this, I have been working on some tooling to flag typos in
new commits. Is that something we'd ever want to automate?

Regards,
Jacob


v01_fix_typos.patch
Description: Binary data

Re: Extend postgres_fdw_get_connections to return remote backend pid

[Fujii Masao]

On 2025/02/28 21:23, Sagar Shedge wrote:

Hi Fujii,

Thanks for updates.
Looks good to me. We can proceed with latest potch.


Thanks for the review! I've pushed the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Change log level for notifying hot standby is waiting non-overflowed snapshot

[Fujii Masao]

On 2025/02/03 22:35, torikoshia wrote:

Hi,

When a hot standby is restarted in a state where subtransactions have 
overflowed, it may become inaccessible:

   $ psql: error: connection to server at "localhost" (::1), port 5433 failed: 
FATAL:  the database system is not yet accepting connections
   DETAIL:  Consistent recovery state has not been yet reached.


Could you share the steps to reproduce this situation?



However, the log message that indicates the cause of this issue seems to be 
only output at the DEBUG1 level:

   elog(DEBUG1,
    "recovery snapshot waiting for non-overflowed snapshot or "
    "until oldest active xid on standby is at least %u (now %u)",
    standbySnapshotPendingXmin,
    running->oldestRunningXid);

I believe this message would be useful not only for developers but also for 
users.


Isn't this log message too difficult for most users? It seems to
describe PostgreSQL's internal mechanisms, making it hard
for users to understand the issue and what actions to take.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Allow table AMs to define their own reloptions

[Yura Sokolov]

02.03.2025 16:23, Julien Tachoires пишет:
> On Sun, Mar 02, 2025 at 09:56:41AM +0100, Julien Tachoires wrote:
>> With the help of the new TAM routine 'relation_options', table access 
>> methods can with this patch define their own reloptions 
>> parser/validator.
>>
>> These reloptions can be set via the following commands:
>> 1. CREATE TABLE ... USING table_am
>>WITH (option1='value1', option2='value2');
>> 2. ALTER TABLE ...
>>SET (option1 'value1', option2 'value2');
>> 3. ALTER TABLE ... SET ACCESS METHOD table_am
>>OPTIONS (option1 'value1', option2 'value2');
>>
>> When changing table's access method, the settings inherited from the 
>> former TAM can be dropped (if not supported by the new TAM) via: DROP 
>> option, or, updated via: SET option 'value'.
>>
>> Currently, tables using different TAMs than heap are able to use heap's 
>> reloptions (fillfactor, toast_tuple_target, etc...). With this patch 
>> applied, this is not the case anymore: if the TAM needs to have access 
>> to similar settings to heap ones, they have to explicitly define them.
>>
>> The 2nd patch file includes a new test module 'dummy_table_am' which 
>> implements a dummy table access method utilized to exercise TAM 
>> reloptions. This test module is strongly based on what we already have 
>> in 'dummy_index_am'. 'dummy_table_am' provides a complete example of TAM 
>> reloptions definition.
>>
>> This work is directly derived from SadhuPrasad's patch here [2]. Others 
>> attempts were posted here [1] and here [3].
>>
>> [1] 
>> https://www.postgresql.org/message-id/flat/429fb58fa3218221bb17c7bf9e70e1aa6cfc6b5d.camel%40j-davis.com
>> [2] 
>> https://www.postgresql.org/message-id/flat/caff0-cg4kzhdtyhmsonwixnzj16gwzpduxan8yf7pddub+g...@mail.gmail.com
>> [3] 
>> https://www.postgresql.org/message-id/flat/AMUA1wBBBxfc3tKRLLdU64rb.1.1683276279979.Hmail.wuhao%40hashdata.cn
> 
> Please find a new version including minor fixes: 'TAM' terms are
> replaced by 'table AM'

Good day, Julien.

Your forgot another one attempt discussion with patch [1] with alive
commitfest entry [2]

[1] https://postgr.es/m/flat/3766675.7eaCOWfIcx%40thinkpad-pgpro
[2] https://commitfest.postgresql.org/patch/4688/


---
regards
Yura Sokolov aka funny-falcon

Re: Add Postgres module info

[Andrei Lepikhov]

On 17/2/2025 04:00, Michael Paquier wrote:

On Mon, Feb 17, 2025 at 03:41:56AM +0200, Alexander Korotkov wrote:

1) Is it intended to switch all in-core libraries to use PG_MODULE_MAGIC_EXT()?
2) Once we have module version information, it looks natural to
specify the required version for dependant objects, e.g. SQL-funcions
implemented in shared libraries.  For instance,
CREATE FUNCTION ... AS 'MODULE_PATHNAME' LANGUAGE C module_version >= '1.0';
For this, and probably other purposes, it's desirable for version to
be something comparable at SQL level.  Should we add some builtin
analogue of pg_text_semver?


I see that this is just a way for extensions to map to some data
statically stored in the modules themselves based on what I can see at
[1].  Why not.

+   boolisnull[3] = {0,0,0};

Could be a simpler {0}.

Done


-PG_MODULE_MAGIC;
+PG_MODULE_MAGIC_EXT(
+   .name = "auto_explain",
+   .version = "1.0.0"
+);

It does not make sense to me to stick that into into of the contrib
modules officially supported just for the sake of the API.  I'd

Done

suggest to switch in one of the modules of src/test/modules/ that are
loaded with shared_preload_libraries.  A second thing I would suggest
to check is a SQL call with a library loaded via SQL with a LOAD.
test_oat_hooks is already LOAD'ed in a couple of scripts, for example.
For the shared_preload_libraries can, you could choose anything to
prove your point with tests.

Done


+Datum
+module_info(PG_FUNCTION_ARGS)

This should use a "pg_" prefix, should use a plural term as it is a
SRF returning information about all the modules loaded.  Perhaps just
name it to pg_get_modules() and also map it to a new system view?

Sure, done.


Some problems with `git diff --check\` showing up here.

Done


No documentation provided.
Ok, I haven't been sure this idea has a chance to be committed. I will 
introduce the docs in the next version.


--
regards, Andrei LepikhovFrom b7472026bbf2ef49df164492a19d8433cbaa1d12 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" 
Date: Tue, 19 Nov 2024 18:45:36 +0700
Subject: [PATCH v3] Introduce PG_MODULE_MAGIC_EXT macro.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This macro provides dynamically loaded shared libraries (modules) with standard
way to incorporate version (supposedly, defined according to semantic versioning
specification) and name data. The introduced catalogue routine pg_get_modules≈ 
can
be used to find this module by name and check the version. It makes users
independent from file naming conventions.

With a growing number of Postgres core hooks and the introduction of named DSM
segments, the number of modules that don't need to be loaded on startup may
grow fast. Moreover, in many cases related to query tree transformation or
extra path recommendation, such modules might not need database objects except
GUCs - see auto_explain as an example. That means they don't need to execute
the 'CREATE EXTENSION' statement at all and don't have a record in
the pg_extension table. Such a trick provides much flexibility, including
an online upgrade and may become widespread.

In addition, it is also convenient in support to be sure that the installation
(or at least the backend) includes a specific version of the module. Even if
a module has an installation script, it is not rare that it provides
an implementation for a range of UI routine versions. It makes sense to ensure
which specific version of the code is used.

Discussions [1,2] already mentioned module-info stuff, but at that time,
extensibility techniques and extension popularity were low, and it wasn't
necessary to provide that data.

[1] https://www.postgresql.org/message-id/flat/20060507211705.GB3808%40svana.org
[2] 
https://www.postgresql.org/message-id/flat/20051106162658.34c31d57%40thunder.logicalchaos.org
---
 src/backend/utils/fmgr/dfmgr.c| 62 ++-
 src/include/catalog/pg_proc.dat   |  6 ++
 src/include/fmgr.h| 28 -
 .../modules/test_dsa/expected/test_dsa.out|  8 +++
 src/test/modules/test_dsa/sql/test_dsa.sql|  4 ++
 .../test_shm_mq/expected/test_shm_mq.out  |  8 +++
 .../modules/test_shm_mq/sql/test_shm_mq.sql   |  4 ++
 src/test/modules/test_shm_mq/test.c   |  5 +-
 .../modules/test_slru/expected/test_slru.out  | 16 +
 src/test/modules/test_slru/sql/test_slru.sql  |  8 +++
 src/test/modules/test_slru/test_slru.c|  5 +-
 11 files changed, 147 insertions(+), 7 deletions(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 87b233cb887..0ae9b21c2bc 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -21,10 +21,12 @@
 #endif /* !WIN32 */
 
 #include "fmgr.h"
+#include "funcapi.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "stora

Re: per backend WAL statistics

[Michael Paquier]

On Fri, Feb 28, 2025 at 09:26:08AM +, Bertrand Drouvot wrote:
> Also attaching the patch I mentioned up-thread to address some of Rahila's
> comments ([1]): It adds a AuxiliaryPidGetProc() call in 
> pgstat_fetch_stat_backend_by_pid()
> and pg_stat_reset_backend_stats(). I think that fully makes sense since 
> a051e71e28a
> modified pgstat_tracks_backend_bktype() for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> and B_WAL_WRITER.

Okay by me as it makes the code automatically more flexible if
pgstat_tracks_backend_bktype() gets tweaked, including the call of
pgstat_flush_backend() in pgstat_report_wal() so as the WAL writer is
able to report backend stats for its WAL I/O.  Applied this part as of
3f1db99bfabb.

Something that's still not quite right is that the WAL receiver and
the WAL summarizer do not call pgstat_report_wal() at all, so we don't
report much data and we expect these processes to run continuously.
The location where to report stats for the WAL summarizer is simple,
even if the system is aggressive with WAL this is never called more
than a couple of times per seconds, like the WAL writer:

@@ -1541,6 +1542,10 @@ summarizer_read_local_xlog_page(XLogReaderState *state,
  * so we don't tight-loop.
  */
 HandleWalSummarizerInterrupts();
+
+/* report pending statistics to the cumulative stats system */
+pgstat_report_wal(false);
+
 summarizer_wait_for_wal();

At this location, the WAL summarizer would wait as there is no data to
read.  The hot path is when we're reading a block.

The WAL receiver is a different story, because the WaitLatchOrSocket()
call in the main loop of WalReceiverMain() is *very* aggressive, and
it's easy to reach this code dozens of times each millisecond.  In
short, we need to be careful, I think, based on how this is currently
written.  My choice is then this path:
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -583,6 +583,10 @@ WalReceiverMain(const void *startup_data, size_t 
startup_data_len)
  */
 boolrequestReply = false;
 
+/* report pending statistics to the cumulative stats 
system */
+pgstat_report_wal(false);
+
 /*
  * Check if time since last receive from prim

This would update the stats only when the WAL receiver has nothing to
do or if wal_receiver_status_interval is reached, so we're not going
to storm pgstats with updates, still we get some data on a periodic
basis *because* wal_receiver_status_interval would make sure that the
path is taken even if we're under a lot of WAL pressure when sending
feedback messages back to the WAL sender.  Of course this needs a
pretty good comment explaining the choice of this location.  What do
you think?

> It looks like it does not need doc updates. Attached as 0002 as it's somehow
> un-related to this thread (but not sure it deserves it's dedicated thread 
> though).

I'm wondering if we should not lift more the list of processes listed
in pgstat_tracks_backend_bktype() and include B_AUTOVAC_LAUNCHER,
B_STARTUP, B_CHECKPOINTER, B_BG_WRITER at this stage, removing the
entire paragraph.  Not sure if we really have to do that for this
release, but we could look at that separately.

With 3f1db99bfabb in place, wouldn't it be simpler to update
pgstat_report_wal() in v12-0001 so as we use PGSTAT_BACKEND_FLUSH_ALL
with one call of pgstat_flush_backend()?  This saves one call for each
stats flush.
--
Michael


signature.asc
Description: PGP signature

Re: [BUG]: the walsender does not update its IO statistics until it exits

[Michael Paquier]

On Fri, Feb 28, 2025 at 10:39:31AM +, Bertrand Drouvot wrote:
> That sounds a good idea to measure the impact of those extra calls and see
> if we'd need to mitigate the impacts. I'll collect some data.

Thanks.
--
Michael


signature.asc
Description: PGP signature

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Mon, Mar 3, 2025 at 8:19 AM Sutou Kouhei  wrote:
>
> Hi,
>
> In <3191030.1740932...@sss.pgh.pa.us>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Sun, 02 Mar 2025 11:27:20 -0500,
>   Tom Lane  wrote:
>
> >> While review another thread (Emitting JSON to file using COPY TO),
> >> I found the recently committed patches on this thread pass the
> >> CopyFormatOptions struct directly rather a pointer of the struct
> >> as a function parameter of CopyToGetRoutine and CopyFromGetRoutine.
> >
> > Coverity is unhappy about that too:
> >
> > /srv/coverity/git/pgsql-git/postgresql/src/backend/commands/copyto.c: 177 
> > in CopyToGetRoutine()
> > 171   .CopyToOneRow = CopyToBinaryOneRow,
> > 172   .CopyToEnd = CopyToBinaryEnd,
> > 173 };
> > 174
> > 175 /* Return a COPY TO routine for the given options */
> > 176 static const CopyToRoutine *
>  CID 1643911:  Performance inefficiencies  (PASS_BY_VALUE)
>  Passing parameter opts of type "CopyFormatOptions" (size 184 bytes) 
>  by value, which exceeds the low threshold of 128 bytes.
> > 177 CopyToGetRoutine(CopyFormatOptions opts)
> > 178 {
> > 179   if (opts.csv_mode)
> > 180   return &CopyToRoutineCSV;
> >
> > (and likewise for CopyFromGetRoutine).  I realize that these
> > functions aren't called often enough for performance to be an
> > overriding concern, but it still seems like poor style.
> >
> >> Then I took a quick look at the newly rebased patch set and
> >> found Sutou has already fixed this issue.
> >
> > +1, except I'd suggest declaring the parameters as
> > "const CopyFormatOptions *opts".
>
> Thanks for pointing out this (and sorry for missing this in
> our reviews...)!
>
> How about the attached patch?

Looking good, thanks

>
> I'll rebase the v35 patch set after this is fixed.
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: making EXPLAIN extensible

[Srinath Reddy]

Hi Robert,
thanks for working on this and +1 for the idea.
i have reviewed 1,2 patches using 3rd patch(pg_overexplain module) and they
LGTM,will review more the 3rd patch.

Regards,
Srinath Reddy Sadipiralla
EDB:http://www.enterprisedb.com

Re: Coccinelle for PostgreSQL development [1/N]: coccicheck.py

[Mats Kindahl]

On Sat, Jan 18, 2025 at 8:44 PM Mats Kindahl  wrote:

> On Tue, Jan 14, 2025 at 4:19 PM Aleksander Alekseev <
> aleksan...@timescale.com> wrote:
>
>> IMO the best solution would be re-submitting all the patches to this
>> thread. Also please make sure the patchset is registered on the
>> nearest open CF [1] This will ensure that the patchset is built on our
>> CI (aka cfbot [2]) and will not be lost.
>>
>> [1]: https://commitfest.postgresql.org/
>> [2]: http://cfbot.cputube.org/
>>
>>
>
Hi all,

Here is a new set of patches rebased on the latest version of the Postgres.
I decided to just include the semantic patches in each patch since it is
trivial to generate the patch and build using:


ninja coccicheck-patch | patch -d .. -p1 && ninja


I repeat the description from the previous patch set and add comments where
things have changed, but I have also added two semantic patches, which are
described last.

For those of you that are not aware of it: Coccinelle is a tool for pattern
> matching and text transformation for C code and can be used for detection
> of problematic programming patterns and to make complex, tree-wide patches
> easy. It is aware of the structure of C code and is better suited to make
> complicated changes than what is possible using normal text substitution
> tools like Sed and Perl. I've noticed it's been used at a few cases way
> back to fix issues.[1]
>
> Coccinelle have been successfully been used in the Linux project since
> 2008 and is now an established tool for Linux development and a large
> number of semantic patches have been added to the source tree to capture
> everything from generic issues (like eliminating the redundant A in
> expressions like "!A || (A && B)") to more Linux-specific problems like
> adding a missing call to kfree().
>
> Although PostgreSQL is nowhere the size of the Linux kernel, it is
> nevertheless of a significant size and would benefit from incorporating
> Coccinelle into the development. I noticed it's been used in a few cases
> way back (like 10 years back) to fix issues in the PostgreSQL code, but I
> thought it might be useful to make it part of normal development practice
> to, among other things:
>
> - Identify and correct bugs in the source code both during development and
> review.
> - Make large-scale changes to the source tree to improve the code based on
> new insights.
> - Encode and enforce APIs by ensuring that function calls are used
> correctly.
> - Use improved coding patterns for more efficient code.
> - Allow extensions to automatically update code for later PostgreSQL
> versions.
>
> To that end, I created a series of patches to show how it could be used in
> the PostgreSQL tree. It is a lot easier to discuss concrete code and I
> split it up into separate messages since that makes it easier to discuss
> each individual patch. The series contains code to make it easy to work
> with Coccinelle during development and reviews, as well as examples of
> semantic patches that capture problems, demonstrate how to make large-scale
> changes, how to enforce APIs, and also improve some coding patterns.
>
> The first three patches contain the coccicheck.py script and the
> integration with the build system (both Meson and Autoconf).
>
> # Coccicheck Script
>
> It is a re-implementation of the coccicheck script that the Linux kernel
> uses. We cannot immediately use the coccicheck script since it is quite
> closely tied to the Linux source code tree and we need to have something
> that both supports Autoconf and Meson. Since Python seems to be used more
> and more in the tree, it seems to be the most natural choice. (I have no
> strong opinion on what language to use, but think it would be good to have
> something that is as platform-independent as possible.)
>
> The intention is that we should be able to use the Linux semantic patches
> directly, so it supports the "Requires" and "Options" keywords, which can
> be used to require a specific version of spatch(1) and add options to the
> execution of that semantic patch, respectively.
>

I have added support for using multiple jobs similar to how "make -jN"
works. This is also supported by the autoconf and ninja builds


> # Autoconf support
>
> The changes to Autoconf modifies the configure.ac and related files (in
> particular Makefile.global.in). At this point, I have deliberately not
> added support for pgxs so extensions cannot use coccicheck through the
> PostgreSQL installation. This is something that we can add later though.
>
> The semantic patches are expected to live in cocci/ directory under the
> root and the patch uses the pattern cocci/**/*.cocci to find all semantic
> patches. Right now there are no subdirectories for the semantic patches,
> but this might be something we want to add to create different categories
> of scripts.
>
> The coccicheck target is used in the same way as for the Linux kernel,
> that is, to generate and apply all patches suggested by the semantic
> pat

Re: gamma() and lgamma() functions

[Dean Rasheed]

On Thu, 14 Nov 2024 at 22:35, Dean Rasheed  wrote:
>
> On Thu, 14 Nov 2024 at 16:28, Dmitry Koval  wrote:
> >
> >  >SELECT gamma(float8 '1e-320');
> > ERROR:  value out of range: overflow
> >
> >  >SELECT gamma(float8 '0');
> >gamma
> > --
> >   Infinity
> > (1 row)
> >
> > Perhaps it would be logical if the behavior in these cases was the same
> > (either ERROR or 'Infinity')?
>
> In general, I think that having gamma() throw overflow errors is
> useful for spotting real problems in user code.
>

Thinking about this afresh, I remain of the opinion that having the
gamma function throw overflow errors is useful for inputs that are too
large, like gamma(200). But then, if we're going to do that, maybe we
should just do the same for other invalid inputs (zero, negative
integers, and -Inf). That resolves the consistency issue with inputs
very close to zero, and seems like a practical solution.

Attached is an update doing that.

Regards,
Dean
From 4613ddc7267343e74703991267a1adb45eff3403 Mon Sep 17 00:00:00 2001
From: Dean Rasheed 
Date: Sun, 2 Mar 2025 09:23:20 +
Subject: [PATCH v5] Add support for gamma() and lgamma() functions.

These are useful general-purpose math functions which are included in
POSIX and C99, and are commonly included in other math libraries, so
expose them as SQL-callable functions.

Author: Dean Rasheed 
Reviewed-by: Stepan Neretin 
Reviewed-by: Tom Lane 
Reviewed-by: Peter Eisentraut 
Reviewed-by: Dmitry Koval 
Discussion: https://postgr.es/m/caezatcxpgyfjxcirfk9au+fvm0y2ah+2-0wsjx7mo368ysn...@mail.gmail.com
---
 doc/src/sgml/func.sgml   | 38 
 src/backend/utils/adt/float.c| 87 
 src/include/catalog/pg_proc.dat  |  7 +++
 src/test/regress/expected/float8.out | 57 ++
 src/test/regress/sql/float8.sql  | 23 
 5 files changed, 212 insertions(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0e6c534965..143031e03e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1396,6 +1396,27 @@ SELECT NOT(ROW(table.*) IS NOT NULL) FROM TABLE; -- detect at least one null in

   
 
+  
+   
+
+ gamma
+
+gamma ( double precision )
+double precision
+   
+   
+Gamma function
+   
+   
+gamma(0.5)
+1.772453850905516
+   
+   
+gamma(6)
+120
+   
+  
+
   

 
@@ -1436,6 +1457,23 @@ SELECT NOT(ROW(table.*) IS NOT NULL) FROM TABLE; -- detect at least one null in

   
 
+  
+   
+
+ lgamma
+
+lgamma ( double precision )
+double precision
+   
+   
+Natural logarithm of the absolute value of the gamma function
+   
+   
+lgamma(1000)
+5905.220423209181
+   
+  
+
   

 
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 2bc31eabf2..a46ebffae4 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2786,6 +2786,93 @@ derfc(PG_FUNCTION_ARGS)
 }
 
 
+/* == GAMMA FUNCTIONS == */
+
+
+/*
+ *		dgamma			- returns the gamma function of arg1
+ */
+Datum
+dgamma(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	/*
+	 * Handle NaN and Inf cases explicitly.  This simplifies the overflow
+	 * checks on platforms that do not set errno.
+	 */
+	if (isnan(arg1))
+		result = arg1;
+	else if (isinf(arg1))
+	{
+		/* Per POSIX, an input of -Inf causes a domain error */
+		if (arg1 < 0)
+		{
+			float_overflow_error();
+			result = get_float8_nan();	/* keep compiler quiet */
+		}
+		else
+			result = arg1;
+	}
+	else
+	{
+		/*
+		 * Note: the POSIX/C99 gamma function is called "tgamma", not "gamma".
+		 *
+		 * On some platforms, tgamma() will not set errno but just return Inf,
+		 * NaN, or zero to report overflow/underflow; therefore, test those
+		 * cases explicitly (note that, like the exponential function, the
+		 * gamma function has no zeros).
+		 */
+		errno = 0;
+		result = tgamma(arg1);
+
+		if (errno != 0 || isinf(result) || isnan(result))
+		{
+			if (result != 0.0)
+float_overflow_error();
+			else
+float_underflow_error();
+		}
+		else if (result == 0.0)
+			float_underflow_error();
+	}
+
+	PG_RETURN_FLOAT8(result);
+}
+
+
+/*
+ *		dlgamma			- natural logarithm of absolute value of gamma of arg1
+ */
+Datum
+dlgamma(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	/*
+	 * Note: lgamma may not be thread-safe because it may write to a global
+	 * variable signgam, which may not be thread-local. However, this doesn't
+	 * matter to us, since we don't use signgam.
+	 */
+	errno = 0;
+	result = lgamma(arg1);
+
+	/*
+	 * If an ERANGE error occurs, it means there is an overflow.
+	 *
+	 * On some platforms, lgamma() will not set errno but just return infinity
+	 *

Re: Doc: clarify possibility of ephemeral discrepancies between state and wait_event in pg_stat_activity

[Alex Friedman]

discrepancy will look like. What about we do something much more
simplified, such
as the below:

"""
To keep the reporting overhead low, the system does not attempt to synchronize
activity data for a backend. As a result, ephemeral discrepancies may
exist between
the view’s columns.
"""


Yes, I believe it makes sense to make it more generic. Attached v3 with a slight 
tweak:


+in the system. To keep the reporting overhead low, the system does not 
attempt to
+synchronize different aspects of activity data for a backend. As a result, 
ephemeral

+discrepancies may exist between the view's columns.


Best regards,

Alex FriedmanFrom 58de88469f6201ae698ee34debcdec028526a72a Mon Sep 17 00:00:00 2001
From: Alex Friedman 
Date: Wed, 26 Feb 2025 19:59:59 +0200
Subject: [PATCH v3] Clarify possibility of ephemeral discrepancies between
 state and wait_event in pg_stat_activity.

---
 doc/src/sgml/monitoring.sgml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 9178f1d34ef..0e34b3509b8 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1016,7 +1016,9 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
 it may or may not be waiting on some event.  If the 
state
 is active and wait_event is 
non-null, it
 means that a query is being executed, but is being blocked somewhere
-in the system.
+in the system. To keep the reporting overhead low, the system does not 
attempt to
+synchronize different aspects of activity data for a backend. As a result, 
ephemeral
+discrepancies may exist between the view's columns.

   
 
-- 
2.41.0

Re: Improving tracking/processing of buildfarm test failures

[Alexander Lakhin]

Hello hackers,

Please take a look at the February report on buildfarm failures:
# SELECT br, count(*) FROM failures WHERE dt >= '2025-02-01' AND
 dt < '2025-03-01' GROUP BY br;
REL_13_STABLE: 17
REL_14_STABLE: 11
REL_15_STABLE: 7
REL_16_STABLE: 15
REL_17_STABLE: 13
master: 186
-- Total: 249
(Counting test failures only, excluding indent-check, Configure, Build
errors.)


# SELECT COUNT(*) FROM (SELECT DISTINCT issue_link FROM failures WHERE
 dt >= '2025-02-01' AND dt < '2025-03-01');
32


 # SELECT issue_link, count(*) FROM failures WHERE dt >= '2025-02-01' AND
 dt < '2025-03-01' GROUP BY issue_link ORDER BY 2 DESC LIMIT 7;
https://www.postgresql.org/message-id/816167.1740278884%40sss.pgh.pa.us : 32
-- Fixed

https://www.postgresql.org/message-id/976dcc37-b629-490e-a052-a057477d062f%40dunslane.net
 : 26
--Fixed

https://www.postgresql.org/message-id/57988cee-5cc8-44f9-981e-8d62e556f5c8%40gmail.com
 : 21
-- Fixed

https://www.postgresql.org/message-id/1834446.1740588...@sss.pgh.pa.us : 21
-- Fixed

https://www.postgresql.org/message-id/CA%2BTgmoaob_3aNF5S49WFoJs6Rb37rNnd5WH_e9nXAn_jtWWCLQ%40mail.gmail.com
 : 16
-- Fixed

https://www.postgresql.org/message-id/559462.1737760111%40sss.pgh.pa.us : 14
-- Fixed

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=5bf12323b : 14
-- Fixed


# SELECT count(*) FROM failures WHERE dt >= '2025-02-01' AND
 dt < '2025-03-01' AND issue_link IS NULL; -- Unsorted/unhelpful failures
13

Short-lived failures:190

(I've also updated my script to exclude "*-build" failures.)

Best regards,
Alexander Lakhin
Neon (https://neon.tech)#!/bin/bash

TMP=${TMPDIR:-/tmp}
wget "https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=31"; -O 
"$TMP/failures.html"
wget "https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures"; -O 
"$TMP/known-test-failures.html"
sed -E 's/\&max_days/\&max_days/; s/(hours|mins|secs| i) < /\1 \< /; 
s/\ / /g' -i "$TMP/failures.html"
cat << 'EOF' > "$TMP/t.xsl"

http://www.w3.org/1999/XSL/Transform";
  xmlns="http://www.w3.org/1999/xhtml";
  xmlns:xhtml="http://www.w3.org/1999/xhtml";>












EOF

for fl in $(xsltproc "$TMP/t.xsl" "$TMP/failures.html"); do
#echo $fl
if [[ $fl == show_log* ]]; then
sfl=${fl/\&/\&}
grep -q "$sfl" "$TMP/known-test-failures.html" && continue
echo "An unknown failure found: 
https://buildfarm.postgresql.org/cgi-bin/$fl";
wget "https://buildfarm.postgresql.org/cgi-bin/$fl"; -O 
"$TMP/failure-$fl.log"

il=""
if grep -q -Pzo \
'(?s)Details for system "[^"]+" failure at stage pg_amcheckCheck,.*'\
'postgresql:pg_amcheck / pg_amcheck/005_opclass_damage\s+TIMEOUT'\
"$TMP/failure-$fl.log"; then

il="https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#005_opclass_damage.pl_fails_on_Windows_animals_due_to_timeout";

elif grep -q -Pzo \
'(?s)pgsql.build/src/test/isolation/output_iso/regression.diffs<.*'\
'\+isolationtester: canceling step d2a1 after (300|360) seconds\s*\n'\
' step d2a1: <... completed>\s*\n'\
'-  sum\s*\n'\
'--\s*\n'\
'-1\s*\n.*'\
'\+ERROR:  canceling statement due to user request\s*\n'\
' step e1c: COMMIT;'\
"$TMP/failure-$fl.log"; then

il="https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#deadlock-parallel.spec_fails_due_to_timeout_on_jit-enabled_animals";
fi

if [ -n "$il" ]; then
echo "The corresponding issue: $il"
echo
fi
else
echo "Invalid link: $fl"
fi
done

Re: bug when apply fast default mechanism for adding new column over domain with default value

[jian he]

On Sun, Mar 2, 2025 at 7:54 AM Tom Lane  wrote:
>
> jian he  writes:
> > So I duplicated StoreAttrDefault and removed pg_attribute.atthasdef,
> > pg_attrdef related code.
> > and it works fine.
>
> I think the fundamental problem here is that StoreAttrDefault
> shouldn't be involved in this in the first place.  It looks like
> somebody did that in the hopes of avoiding two updates of the
> new pg_attribute row (one to set atthasdef and the other to fill
> attmissingval), but it's just bad code structure.  We should
> take that code out of StoreAttrDefault, not duplicate it, because
> the assumption that the missingval is identical to what goes into
> pg_attrdef is just wrong.
>

in StoreAttrDefault,
i've split missingval related code into StoreAttrMissingVal.

> Also, since the "if (RELKIND_HAS_STORAGE(relkind))" stanza
> in ATExecAddColumn is already calling expression_planner,
> we should be able to avoid doing that twice on the way to
> discovering whether the expression is a constant.
done.


> I kind of feel that StoreAttrMissingVal belongs in heap.c;
> it's got about nothing to do with pg_attrdef.
heap.c seems fine.
From bd79aede786a16e0c105dc28f6a0f7e6f045bed0 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Sun, 2 Mar 2025 17:49:03 +0800
Subject: [PATCH v2 1/1] apply fast default for adding new column over domain
 with default value

split part of logic in StoreAttrDefault to new function: StoreAttrMissingVal.
it will handle: construct attmissingval value and update
pg_attribute.attmissingval, atthasmissing for the specific attnum. it will
optionally update atthasdef.

we use StoreAttrMissingVal to apply fast default mechanism to domain with default value.
discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kqgl+tnvox5lo...@mail.gmail.com
---
 src/backend/catalog/heap.c | 110 +
 src/backend/catalog/pg_attrdef.c   |  82 +--
 src/backend/commands/tablecmds.c   |  26 -
 src/include/catalog/heap.h |   4 +
 src/test/regress/expected/fast_default.out |  49 +
 src/test/regress/sql/fast_default.sql  |  36 +++
 6 files changed, 225 insertions(+), 82 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 956f196fc95..77358a3b4e1 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -57,6 +57,7 @@
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
 #include "common/int.h"
+#include "executor/executor.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -69,6 +70,7 @@
 #include "pgstat.h"
 #include "storage/lmgr.h"
 #include "storage/predicate.h"
+#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -2068,6 +2070,114 @@ SetAttrMissing(Oid relid, char *attname, char *value)
 	table_close(tablerel, AccessExclusiveLock);
 }
 
+/*
+ * StoreAttrMissingVal
+ * Stores the datum value of pg_attribute.attmissingval for the attribute attnum
+ * optionally update atthasdef.  currently this is mainly used within
+ * StoreAttrDefault, but it can be used seperately.
+ *
+ * attnum: The attribute number we are interested into.
+ * expr: The expression to be evaluated, whose resulting datum value is stored
+ * in pg_attribute.attmissingval.
+ * add_column_mode: it has the same meaning as in StoreAttrDefault. should pass
+ * as true is not used in the context of StoreAttrDefault.
+ * stored_default: If true, pg_attribute.atthasdef is updated to true.
+ * att_generated: If not NULL, it will set as pg_attribute.ttgenerated for attnum.
+ */
+void
+StoreAttrMissingVal(Relation rel, AttrNumber attnum, Node *expr,
+	bool add_column_mode, bool stored_default, char *att_generated)
+{
+	Relation	attrrel;
+	HeapTuple	atttup;
+	Form_pg_attribute attStruct;
+	Form_pg_attribute defAttStruct;
+	char		attgenerated;
+
+	/*
+	 * Update the pg_attribute entry for the column to show that we store attmissingval
+	 * on it.
+	 */
+	attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+	atttup = SearchSysCacheCopy2(ATTNUM,
+ ObjectIdGetDatum(RelationGetRelid(rel)),
+ Int16GetDatum(attnum));
+	if (!HeapTupleIsValid(atttup))
+		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+			 attnum, RelationGetRelid(rel));
+	attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
+	attgenerated = attStruct->attgenerated;
+	if (att_generated)
+		*att_generated = attgenerated;
+
+	if (!attStruct->atthasdef)
+	{
+		ExprState  *exprState;
+		Expr	   *expr2 = (Expr *) expr;
+		EState	   *estate = NULL;
+		ExprContext *econtext;
+		Datum		valuesAtt[Natts_pg_attribute] = {0};
+		bool		nullsAtt[Natts_pg_attribute] = {0};
+		bool		replacesAtt[Natts_pg_attribute] = {0};
+		Datum		missingval = (Datum) 0;
+		bool		missingIsNull = true;
+
+		if (stored_default)
+		{
+			valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
+			replacesAtt[Anum_pg_attribute_att

Options to control remote transactions’ access/deferrable modes in postgres_fdw

[Etsuro Fujita]

Hi,

postgres_fdw opens remote transactions in read/write mode in a local
transaction even if the local transaction is read-only.  I noticed
that this leads to surprising behavior like this:

CREATE TABLE test (a int);
CREATE FUNCTION testfunc() RETURNS int LANGUAGE SQL AS 'INSERT INTO
public.test VALUES (1) RETURNING *';
CREATE VIEW testview(a) AS SELECT testfunc();
CREATE FOREIGN TABLE testft (a int) SERVER loopback OPTIONS
(table_name 'testview');

START TRANSACTION READ ONLY;
SELECT * FROM testft;
 a
---
 1
(1 row)

COMMIT;
SELECT * FROM test;
 a
---
 1
(1 row)

The transaction is declared as READ ONLY, but the INSERT statement is
successfully executed in the remote side.

To avoid that, I would like to propose a server option,
inherit_read_only, to open the remote transactions in read-only mode
if the local transaction is read-only.

I would also like to propose a server option, inherit_deferrable, to
open the remote transactions in deferrable mode if the local
transaction is deferrable.

Attached is a small patch for these options.  I will add this to the
March commitfest as it is still open.

Best regards,
Etsuro Fujita


Inherit-xact-properties-in-postgres-fdw.patch
Description: Binary data

Re: Considering fractional paths in Append node

[Alexander Korotkov]

Hi, Andrei!

On Fri, Dec 6, 2024 at 10:13 AM Andrei Lepikhov  wrote:
>
> On 12/6/24 13:48, Andrei Lepikhov wrote:
> > On 11/2/24 01:18, Nikita Malakhov wrote:
> >> I've corrected failing test and created a patch at Commitfest:
> >> https://commitfest.postgresql.org/51/5361/  >> commitfest.postgresql.org/51/5361/>
> > I have played around with this feature, which looks promising for such a
> > tiny change. It provides a 'bottom boundary' recommendation for
> > appending subpaths, participating in the 'fractional branch' of paths.
> > As I see it works consistently with the plans, created for plain tables
> > filled with similar data.
> > According to the proposal to change SeqScan logic, IMO, Andy is right.
> > But it is a separate improvement because it wouldn't work in the case of
> > LIMIT 10 or 100, as the newly added regression tests demonstrate.
> > I think this feature gives sensible profit for partitionwise paths.
> > Pushing this knowledge into subpaths could help postgres_fdw to reduce
> > network traffic.
> >
> See the attached patch: regression tests added; *_ext function removed -
> I think we wouldn't back-patch it into minor releases.

Thank you for revising this patch.  I've a couple of questions for you.
1. You revise get_cheapest_fractional_path() to reject parametrized
paths in all the cases.  Are you sure this wouldn't affect other
callers of this function?
2. As usage of root->tuple_fraction RelOptInfo it has been criticized,
do you think we could limit this to some simple cases?  For instance,
check that RelOptInfo is the final result relation for given root.

Links.
https://www.postgresql.org/message-id/871q0fvbje.fsf%40163.com

--
Regards,
Alexander Korotkov
Supabase

Re: Considering fractional paths in Append node

[Alexander Korotkov]

Hi, Andy!

On Fri, Oct 18, 2024 at 3:55 AM Andy Fan  wrote:
>
> Nikita Malakhov  writes:
>
>
> > The effect is easily seen in one of standard PG tests:
> > Vanilla (current master):
> > explain analyze
> > select t1.unique1 from tenk1 t1
> > inner join tenk2 t2 on t1.tenthous = t2.tenthous and t2.thousand = 0
> >union all
> > (values(1)) limit 1;
> >   QUERY PLAN
> >
> > --
> >
> >  Limit  (cost=0.00..219.55 rows=1 width=4) (actual time=6.309..6.312 rows=1 
> > loops=1)
> >->  Append  (cost=0.00..2415.09 rows=11 width=4) (actual 
> > time=6.308..6.310 rows=1 loops=1)
> >  ->  Nested Loop  (cost=0.00..2415.03 rows=10 width=4) (actual 
> > time=6.307..6.308 rows=1 loops=1)
> >Join Filter: (t1.tenthous = t2.tenthous)
> >Rows Removed by Join Filter: 4210
> >->  Seq Scan on tenk1 t1  (cost=0.00..445.00 rows=1 
> > width=8) (actual time=0.004..0.057 rows=422
> > loops=1)
> >->  Materialize  (cost=0.00..470.05 rows=10 width=4) (actual 
> > time=0.000..0.014 rows=10 loops=422)
> >  Storage: Memory  Maximum Storage: 17kB
> >  ->  Seq Scan on tenk2 t2  (cost=0.00..470.00 rows=10 
> > width=4) (actual time=0.076..5.535 rows=10
> > loops=1)
> >Filter: (thousand = 0)
> >Rows Removed by Filter: 9990
> >  ->  Result  (cost=0.00..0.01 rows=1 width=4) (never executed)
> >  Planning Time: 0.324 ms
> >  Execution Time: 6.336 ms
> > (14 rows)
> >
> > Patched, the same test:
> > explain analyze
> > select t1.unique1 from tenk1 t1
> > inner join tenk2 t2 on t1.tenthous = t2.tenthous and t2.thousand = 0
> >union all
> > (values(1)) limit 1;
> > QUERY 
> > PLAN
> >
> > --
> >
> >  Limit  (cost=0.29..126.00 rows=1 width=4) (actual time=0.105..0.106 rows=1 
> > loops=1)
> >->  Append  (cost=0.29..1383.12 rows=11 width=4) (actual 
> > time=0.104..0.105 rows=1 loops=1)
> >  ->  Nested Loop  (cost=0.29..1383.05 rows=10 width=4) (actual 
> > time=0.104..0.104 rows=1 loops=1)
> >->  Seq Scan on tenk2 t2  (cost=0.00..470.00 rows=10 
> > width=4) (actual time=0.076..0.076 rows=1 loops=1)
> >  Filter: (thousand = 0)
> >  Rows Removed by Filter: 421
> >->  Index Scan using tenk1_thous_tenthous on tenk1 t1  
> > (cost=0.29..91.30 rows=1 width=8) (actual
> > time=0.026..0.026 rows=1 loops=1)
> >  Index Cond: (tenthous = t2.tenthous)
> >  ->  Result  (cost=0.00..0.01 rows=1 width=4) (never executed)
> >  Planning Time: 0.334 ms
> >  Execution Time: 0.130 ms
> > (11 rows)
> >
>
> This is a nice result. After some more thoughts, I'm feeling the startup
> cost calculation on seq scan looks a more important one to address.
>
> Bad Plan: Append  (cost=0.00..2415.09 ..) shows us the "startup cost" is 0.
> Good plan: Append  (cost=0.29..1383.12 ..) show us the "startup cost" is
> 0.29.
>
> The major reason of this is we calculate the "startup cost" for
> "SeqScan" and "Index scan" with different guidances. For the "Index
> scan", the startup cost is "the cost to retrieve the first tuple",
> however for "SeqScan", it is not, as we can see the startup cost for
> query "SELECT * FROM tenk2 WHERE thousand = 0" has a 0 startup_cost.
>
> In my understading, "startup cost" means the cost to retrieve the first
> tuple *already*, but at [1], Tom said:
>
> """
> I think that things might work out better if we redefined the startup
> cost as "estimated cost to retrieve the first tuple", rather than its
> current very-squishy definition as "cost to initialize the scan".
> """
>
> The above statement makes me confused. If we take the startup cost as
> cost to retrieve cost for the first tuple, we can do the below quick hack,
>
> @@ -355,8 +355,8 @@ cost_seqscan(Path *path, PlannerInfo *root,
> }
>
> path->disabled_nodes = enable_seqscan ? 0 : 1;
> -   path->startup_cost = startup_cost;
> path->total_cost = startup_cost + cpu_run_cost + disk_run_cost;
> +   path->startup_cost = startup_cost +   (cpu_run_cost + disk_run_cost) 
> * (1 - path->rows / baserel->tuples);
>  }

You can check I've already proposed similar change years before.
https://www.postgresql.org/message-id/CAPpHfdvfDAXPXhFQT3Ww%3DkZ4tpyAsD07_oz8-fh0%3Dp6eckEBKQ%40mail.gmail.com
It appears that according to the current model startup_cost is not the
cost of the first tuple.  It should be the cost of preparation work,
while extraction of tuples should be distributed uniformly through
tot

Allow table AMs to define their own reloptions

[Julien Tachoires]

Hi,

With the help of the new TAM routine 'relation_options', table access 
methods can with this patch define their own reloptions 
parser/validator.

These reloptions can be set via the following commands:
1. CREATE TABLE ... USING table_am
   WITH (option1='value1', option2='value2');
2. ALTER TABLE ...
   SET (option1 'value1', option2 'value2');
3. ALTER TABLE ... SET ACCESS METHOD table_am
   OPTIONS (option1 'value1', option2 'value2');

When changing table's access method, the settings inherited from the 
former TAM can be dropped (if not supported by the new TAM) via: DROP 
option, or, updated via: SET option 'value'.

Currently, tables using different TAMs than heap are able to use heap's 
reloptions (fillfactor, toast_tuple_target, etc...). With this patch 
applied, this is not the case anymore: if the TAM needs to have access 
to similar settings to heap ones, they have to explicitly define them.

The 2nd patch file includes a new test module 'dummy_table_am' which 
implements a dummy table access method utilized to exercise TAM 
reloptions. This test module is strongly based on what we already have 
in 'dummy_index_am'. 'dummy_table_am' provides a complete example of TAM 
reloptions definition.

This work is directly derived from SadhuPrasad's patch here [2]. Others 
attempts were posted here [1] and here [3].

[1] 
https://www.postgresql.org/message-id/flat/429fb58fa3218221bb17c7bf9e70e1aa6cfc6b5d.camel%40j-davis.com
[2] 
https://www.postgresql.org/message-id/flat/caff0-cg4kzhdtyhmsonwixnzj16gwzpduxan8yf7pddub+g...@mail.gmail.com
[3] 
https://www.postgresql.org/message-id/flat/AMUA1wBBBxfc3tKRLLdU64rb.1.1683276279979.Hmail.wuhao%40hashdata.cn

-- 
Julien Tachoires
>From 8968bb1cf92e373523377c79ff42e76dc9fc20ed Mon Sep 17 00:00:00 2001
From: Julien Tachoires 
Date: Sat, 1 Mar 2025 17:59:49 +0100
Subject: [PATCH 1/2] Allow table AMs to define their own reloptions

With the help of the new routine 'relation_options', table access
methods can now define their own reloptions.

These options can be set via the following commands:
1. CREATE TABLE ... USING table_am
   WITH (option1='value1', option2='value2');
2. ALTER TABLE ...
   SET (option1 'value1', option2 'value2');
3. ALTER TABLE ... SET ACCESS METHOD table_am
   OPTIONS (option1 'value1', option2 'value2');

When changing table's access method, the settings from the former
TAM can be dropped (if not supported by the new TAM) via:
DROP option, or, updated via: SET option 'value'.

Before this commit, tables using different TAMs than heap were able
to use heap's reloptions (fillfactor, toast_tuple_target, etc...).
Now, this is not the case anymore: if the TAM needs to have access
to settings similar to heap ones, they must explicitly define them.

This work is directly derived from SadhuPrasad's patch named:
v4-0001-PATCH-V4-Per-table-storage-parameters-for-TableAM.patch
---
 doc/src/sgml/ref/alter_table.sgml|  13 +-
 doc/src/sgml/ref/create_table.sgml   |   3 +-
 src/backend/access/common/reloptions.c   |  66 -
 src/backend/access/heap/heapam_handler.c |   2 +
 src/backend/commands/foreigncmds.c   |   2 +-
 src/backend/commands/tablecmds.c | 180 ---
 src/backend/parser/gram.y|   9 ++
 src/backend/postmaster/autovacuum.c  |  18 ++-
 src/backend/utils/cache/relcache.c   |  11 +-
 src/include/access/reloptions.h  |   6 +-
 src/include/access/tableam.h |  10 ++
 src/include/commands/defrem.h|   1 +
 12 files changed, 286 insertions(+), 35 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8e56b8e59b0..e38200e20d2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -76,7 +76,7 @@ ALTER TABLE [ IF EXISTS ] name
 CLUSTER ON index_name
 SET WITHOUT CLUSTER
 SET WITHOUT OIDS
-SET ACCESS METHOD { new_access_method | DEFAULT }
+SET ACCESS METHOD { new_access_method | DEFAULT } [ OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ] ) ]
 SET TABLESPACE new_tablespace
 SET { LOGGED | UNLOGGED }
 SET ( storage_parameter [= value] [, ... ] )
@@ -734,7 +734,7 @@ WITH ( MODULUS numeric_literal, REM

 

-SET ACCESS METHOD
+SET ACCESS METHOD { new_access_method | DEFAULT } [ OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ] ) ]
 
  
   This form changes the access method of the table by rewriting it
@@ -752,6 +752,15 @@ WITH ( MODULUS numeric_literal, REM
   causing future partitions to default to
   default_table_access_method.
  
+ 
+  Specifying OPTIONS allows to change options for
+  the table when changing the table access method.
+  ADD, SET, and
+  DROP specify the action to be performed.
+  ADD is assumed if no operation is explicitly
+  specified.  Option names must be unique; names and values are also
+  validated using 

Re: [PATCH] Add regression tests of ecpg command notice (error / warning)

[Fujii Masao]

On 2025/03/01 19:45, Ryo Kanbayashi wrote:

+program_help_ok('ecpg');
+program_version_ok('ecpg');
+program_options_handling_ok('ecpg');
+command_fails(['ecpg'], 'ecpg without arguments fails');

These checks seem unnecessary in 002 since they're already covered in 001.


I reflected above.


Thanks for updating the patch!

I've made some minor fixes and cosmetic adjustments.
The updated patch is attached.

Unless there are any objections, I'll commit it.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From 3d06f586d1f9f4760f8259bc10c11f2152c3266f Mon Sep 17 00:00:00 2001
From: Fujii Masao 
Date: Mon, 3 Mar 2025 09:24:02 +0900
Subject: [PATCH v6] ecpg: Add TAP test for the ecpg command.

This commit adds a TAP test to verify that the ecpg command correctly
detects unsupported or disallowed statements in input files and reports
the appropriate error or warning messages.

This test helps catch bugs like the one introduced in commit 3d009e45bd,
which broke ecpg's handling of unsupported COPY FROM STDIN statements,
later fixed by commit 94b914f601b.

Author: Ryo Kanbayashi 
Reviewed-by: Fujii Masao 
Discussion: 
https://postgr.es/m/canon0ezomyxa1m-quds1uequq6fnki6+ggigucgr9tm2r78...@mail.gmail.com
---
 src/interfaces/ecpg/preproc/Makefile  |  3 ++
 src/interfaces/ecpg/preproc/meson.build   | 13 ++
 .../ecpg/preproc/t/001_ecpg_err_warn_msg.pl   | 40 ++
 .../t/002_ecpg_err_warn_msg_informix.pl   | 22 ++
 .../ecpg/preproc/t/err_warn_msg.pgc   | 42 +++
 .../ecpg/preproc/t/err_warn_msg_informix.pgc  | 18 
 6 files changed, 138 insertions(+)
 create mode 100644 src/interfaces/ecpg/preproc/t/001_ecpg_err_warn_msg.pl
 create mode 100644 
src/interfaces/ecpg/preproc/t/002_ecpg_err_warn_msg_informix.pl
 create mode 100644 src/interfaces/ecpg/preproc/t/err_warn_msg.pgc
 create mode 100644 src/interfaces/ecpg/preproc/t/err_warn_msg_informix.pgc

diff --git a/src/interfaces/ecpg/preproc/Makefile 
b/src/interfaces/ecpg/preproc/Makefile
index 84199a9a5d0..d0e3852a878 100644
--- a/src/interfaces/ecpg/preproc/Makefile
+++ b/src/interfaces/ecpg/preproc/Makefile
@@ -81,6 +81,9 @@ ecpg_keywords.o: ecpg_kwlist_d.h
 c_keywords.o: c_kwlist_d.h
 keywords.o: $(top_srcdir)/src/include/parser/kwlist.h
 
+check:
+   $(prove_check)
+
 install: all installdirs
$(INSTALL_PROGRAM) ecpg$(X) '$(DESTDIR)$(bindir)'
 
diff --git a/src/interfaces/ecpg/preproc/meson.build 
b/src/interfaces/ecpg/preproc/meson.build
index bfd0ed2efb4..01f2ac671ec 100644
--- a/src/interfaces/ecpg/preproc/meson.build
+++ b/src/interfaces/ecpg/preproc/meson.build
@@ -86,3 +86,16 @@ ecpg_exe = executable('ecpg',
 ecpg_targets += ecpg_exe
 
 subdir('po', if_found: libintl)
+
+tests += {
+  'name': 'ecpg',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'tap': {
+'tests': [
+  't/001_ecpg_err_warn_msg.pl',
+  't/002_ecpg_err_warn_msg_informix.pl',
+],
+'deps': ecpg_exe,
+  },
+}
\ No newline at end of file
diff --git a/src/interfaces/ecpg/preproc/t/001_ecpg_err_warn_msg.pl 
b/src/interfaces/ecpg/preproc/t/001_ecpg_err_warn_msg.pl
new file mode 100644
index 000..a18e09e6ee8
--- /dev/null
+++ b/src/interfaces/ecpg/preproc/t/001_ecpg_err_warn_msg.pl
@@ -0,0 +1,40 @@
+
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+program_help_ok('ecpg');
+program_version_ok('ecpg');
+program_options_handling_ok('ecpg');
+command_fails(['ecpg'], 'ecpg without arguments fails');
+
+# Test that the ecpg command correctly detects unsupported or disallowed
+# statements in the input file and reports the appropriate error or
+# warning messages.
+command_checks_all(
+   [ 'ecpg', 't/err_warn_msg.pgc' ],
+   3,
+   [qr//],
+   [
+   qr/ERROR: AT option not allowed in CONNECT statement/,
+   qr/ERROR: AT option not allowed in DISCONNECT statement/,
+   qr/ERROR: AT option not allowed in SET CONNECTION statement/,
+   qr/ERROR: AT option not allowed in TYPE statement/,
+   qr/ERROR: AT option not allowed in WHENEVER statement/,
+   qr/ERROR: AT option not allowed in VAR statement/,
+   qr/WARNING: COPY FROM STDIN is not implemented/,
+   qr/ERROR: using variable "cursor_var" in different declare 
statements is not supported/,
+   qr/ERROR: cursor "duplicate_cursor" is already defined/,
+   qr/ERROR: SHOW ALL is not implemented/,
+   qr/WARNING: no longer supported LIMIT/,
+   qr/WARNING: cursor "duplicate_cursor" has been declared but not 
opened/,
+   qr/WARNING: cursor "duplicate_cursor" has been declared but not 
opened/,
+   qr/WARNING: cursor ":cursor_var" has been declared but not 
opene

Re: [Patch] add new parameter to pg_replication_origin_session_setup

[Amit Kapila]

On Thu, Jan 9, 2025 at 3:26 AM Euler Taveira  wrote:
>
> On Thu, Aug 15, 2024, at 5:53 PM, Doruk Yilmaz wrote:
>
> Hello again,
>
> On Tue, Aug 13, 2024 at 12:48 AM Euler Taveira  wrote:
> > I'm curious about your use case. Is it just because the internal function 
> > has a
> > different signature or your tool is capable of apply logical replication 
> > changes
> > in parallel using the SQL API?
>
> The latter is correct, it applies logical replication changes in parallel.
> Since multiple connections may commit, we need all of them to be able
> to advance the replication origin.
>

To use replication_origin by multiple processes, one must maintain the
commit order as we do internally by allowing the leader process to
wait for the parallel worker to finish the commit. See comments atop
replorigin_session_setup(). Now, we could expose the pid parameter as
proposed by the patch after documenting the additional requirements,
but I am afraid that users may directly start using the API without
following the commit order principle, which can lead to incorrect
replication. So, isn't it better to do something to avoid the misuse
of this feature before exposing it?

-- 
With Regards,
Amit Kapila.

Re: Get rid of WALBufMappingLock

[Alexander Korotkov]

On Fri, Feb 28, 2025 at 3:44 PM Michael Paquier  wrote:
>
> On Fri, Feb 28, 2025 at 02:13:23PM +0100, Álvaro Herrera wrote:
> > On 2025-Feb-28, Michael Paquier wrote:
> >> Saying that, I have also done similar tests with your v12 for a couple
> >> of hours and this looks stable under installcheck-world.  I can see
> >> that you've reworked quite a bit the surroundings of InitializedFrom
> >> in this one.  If you apply that once again at some point, the
> >> buildfarm will be judge in the long-term, but I am rather confident by
> >> saying that the situation looks better here, at least.
> >
> > Heh, no amount of testing can prove lack of bugs; but for sure "it looks
> > different now, so it must be correct" must be the weakest proof of
> > correctness I've heard of!
>
> Err, okay.  I did use the word "stable" with tests rather than
> "correct", and I implied upthread that I did not check the correctness
> nor the internals of the patch.  If my words held the meaning you
> are implying, well, my apologies for the confusion, I guess.  I only
> tested the patch and it was stable while I've noticed a few diffs with
> the previous version, but I did *not* check its internals at all, nor
> do I mean that I endorse its logic.  I hope that's clear now.

Got it.  Michael, thank you very much for your help.

--
Regards,
Alexander Korotkov
Supabase

Re: Get rid of WALBufMappingLock

[Alexander Korotkov]

On Fri, Feb 28, 2025 at 3:13 PM Álvaro Herrera  wrote:
> On 2025-Feb-28, Michael Paquier wrote:
>
> > Saying that, I have also done similar tests with your v12 for a couple
> > of hours and this looks stable under installcheck-world.  I can see
> > that you've reworked quite a bit the surroundings of InitializedFrom
> > in this one.  If you apply that once again at some point, the
> > buildfarm will be judge in the long-term, but I am rather confident by
> > saying that the situation looks better here, at least.
>
> Heh, no amount of testing can prove lack of bugs; but for sure "it looks
> different now, so it must be correct" must be the weakest proof of
> correctness I've heard of!

Michael just volunteered to help Yura and me with testing.  He wan't
intended to be reviewer.  And he reported that tests looks much more
stable now.  I think he is absolutely correct with this.

--
Regards,
Alexander Korotkov
Supabase

Re: Options to control remote transactions’ access/deferrable modes in postgres_fdw

[Etsuro Fujita]

On Sun, Mar 2, 2025 at 12:44 PM Etsuro Fujita  wrote:
> Attached is a small patch for these options.  I will add this to the
> March commitfest as it is still open.

The CF was changed to in-progress just before, so I added it to the next CF.

Best regards,
Etsuro Fujita

Re: bug when apply fast default mechanism for adding new column over domain with default value

[Tom Lane]

jian he  writes:
> On Sun, Mar 2, 2025 at 7:54 AM Tom Lane  wrote:
>> I think the fundamental problem here is that StoreAttrDefault
>> shouldn't be involved in this in the first place.

> i've split missingval related code into StoreAttrMissingVal.

Hm, this does nothing to improve the modularity of the affected
code; if anything it's worse than before.  (Fools around for
awhile...)  I had something more like the attached in mind.
The idea here is to centralize the control of whether we are
storing a missingval or doing a table rewrite in ATExecAddColumn.
StoreAttrMissingVal has nothing to do with pg_attrdef nor does
StoreAttrDefault have anything to do with attmissingval.

I looked briefly at determining the presence of a default
earlier so we could avoid the extra update of pg_attribute
when both those changes need to happen, but I concluded that
it'd take more refactoring than it's worth.  The problem is
the logic way down inside AddRelationNewConstraints that
checks for the eventually-cooked default expression just
being a null constant.

regards, tom lane

From 97c896cefa8a0ea4203974b40bfed6008213b95a Mon Sep 17 00:00:00 2001
From: Tom Lane 
Date: Sun, 2 Mar 2025 15:34:05 -0500
Subject: [PATCH v3 1/2] Fix broken handling of domains in atthasmissing logic.

If a domain type has a default, adding a column of that type (without
any explicit DEFAULT clause) failed to install the domain's default
value in existing rows, instead leaving the new column null.  This
is unexpected, and it used to work correctly before v11.  The cause
is confusion in the atthasmissing mechanism about which default value
to install: we'd only consider installing an explicitly-specified
default, and then we'd decide that no table rewrite is needed.

To fix, take the responsibility for filling attmissingval out of
StoreAttrDefault, and instead put it into ATExecAddColumn's existing
logic that derives the correct value to fill the new column with.
Also, centralize the logic that determines the need for
default-related table rewriting there, instead of spreading it over
four or five places.

In the back branches, we'll leave the attmissingval-filling code
in StoreAttrDefault even though it's now dead, for fear that some
extension may be depending on that functionality to exist there.
A separate HEAD-only patch will clean up the now-useless code.

Reported-by: jian he 
Author: jian he 
Author: Tom Lane 
Discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kqgl+tnvox5lo...@mail.gmail.com
Backpatch-through: 13
---
 src/backend/catalog/heap.c | 71 --
 src/backend/catalog/pg_attrdef.c   |  6 ++
 src/backend/commands/tablecmds.c   | 85 +++---
 src/include/catalog/heap.h |  5 +-
 src/test/regress/expected/fast_default.out | 65 +
 src/test/regress/sql/fast_default.sql  | 44 +++
 6 files changed, 242 insertions(+), 34 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 956f196fc95..d28ed3eb396 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -69,6 +69,7 @@
 #include "pgstat.h"
 #include "storage/lmgr.h"
 #include "storage/predicate.h"
+#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -2008,6 +2009,69 @@ RelationClearMissing(Relation rel)
 	table_close(attr_rel, RowExclusiveLock);
 }
 
+/*
+ * StoreAttrMissingVal
+ *
+ * Set the missing value of a single attribute.
+ */
+void
+StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+	Datum missingval, bool missingIsNull)
+{
+	Datum		valuesAtt[Natts_pg_attribute] = {0};
+	bool		nullsAtt[Natts_pg_attribute] = {0};
+	bool		replacesAtt[Natts_pg_attribute] = {0};
+	Relation	attrrel;
+	Form_pg_attribute attStruct;
+	HeapTuple	atttup,
+newtup;
+
+	/* This is only supported for plain tables */
+	Assert(rel->rd_rel->relkind == RELKIND_RELATION);
+
+	/* Update the pg_attribute row */
+	attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+
+	atttup = SearchSysCache2(ATTNUM,
+			 ObjectIdGetDatum(RelationGetRelid(rel)),
+			 Int16GetDatum(attnum));
+	if (!HeapTupleIsValid(atttup))	/* shouldn't happen */
+		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+			 attnum, RelationGetRelid(rel));
+	attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
+
+	if (missingIsNull)
+	{
+		/* if the default value is NULL, just store a NULL array */
+		missingval = (Datum) 0;
+	}
+	else
+	{
+		/* otherwise make a one-element array of the value */
+		missingval = PointerGetDatum(construct_array(&missingval,
+	 1,
+	 attStruct->atttypid,
+	 attStruct->attlen,
+	 attStruct->attbyval,
+	 attStruct->attalign));
+	}
+
+	valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(!missingIsNull);
+	replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
+
+	valu

Re: Cannot find a working 64-bit integer type on Illumos

[Thomas Munro]

On Tue, Dec 10, 2024 at 3:02 PM Thomas Munro  wrote:
> On Thu, Dec 5, 2024 at 12:16 PM Tom Lane  wrote:
> > Now you already snuck the camel's nose under the
> > tent by including stdint.h there, and maybe these additional headers
> > wouldn't do any further damage.
>
> Even though we fixed the immediate issue (thanks), this comment stayed
> with me.  I did that because I didn't want to change any interfaces at
> the same time as the  retrofit, but I agree that it feels a
> bit odd hidden in there, and doesn't appear to conform to
> postgres_ext.h's self-description.  Stepping back, and I realise it's
> difficult to answer with certainty, I wonder why anyone would ever
> want to use postgres_ext.h directly for the definition of pg_int64
> *without* being a user of libpq-fe.h.  I can't find any references to
> pg_int64 (excluding forks of our code) on github; there are a few
> things like proxies and suchlike that include postgres_ext.h for other
> things, mostly bogusly (they also include libpq-fe.h, or they say they
> want NAMEDATALEN, which isn't there anymore).
>
> We have just three lo_*64() functions using that type and then
> pg_usec_time_t.  Seems like a very narrow usage that hasn't spread,
> likely only used to receive arguments, and really quite specific to
> libpq-fe.h and not one of the "fundamental Postgres declarations".
> Maybe we should consider moving #include  into libpq-fe.h?
>
> And if we included  overtly, rather than covertly in
> postgres_ext.h, why would we still want a third name for int64_t?  We
> could change the three lo_*64() declarations to use the standard type
> directly, but keep the historical typedef marked deprecated.
>
> > But I don't see a strong argument to
> > change long-standing external APIs any more than we absolutely must.
>
> So perhaps you'll hate that idea then.  I wonder if you'll hate it
> more than keeping the #include in postgres_ext.h, hence putting the
> idea forward!

Does anyone else have thoughts about this arguable leftover quirk from
the  refactoring work?  In brief, shouldn't libpq-fe.h
include  directly, and use int64_t explicitly, instead of
doing it "secretly" in another header that came about because of
historical namespace pollution concerns, now gone?  We require you to
have a 64 bit integer type, we require C99, C99 requires int64_t to be
defined if you have a 64 bit type, and there doesn't seem to be any
reason to want pg_int64 other than to use these large object functions
in libpq-fe.h.  The current arrangement feels a bit obfuscated,
leading to the patch in the previous email.  Adding Ishii-san who
introduced the three uses of pg_int64 to libpq-fe.h in 461ef73f.

Re: doc: Mention clock synchronization recommendation for hot_standby_feedback

[Jakub Wartak]

Hi Amit,

On Mon, Mar 3, 2025 at 6:26 AM Amit Kapila  wrote:
[..]

OK, sure.

> How about something like: "Note that if the clock on standby is moved
> ahead or backward, the feedback message may not be sent at the
> required interval. This can lead to prolonged risk of not removing
> dead rows on primary for extended periods as the feedback mechanism is
> based on timestamp."

Sure thing. I've just added '(..) In the extreme cases this can..' as
it is pretty rare to hit it. Patch attached.

-J.


v2-0001-doc-Mention-clock-synchronization-recommendation-.patch
Description: Binary data

RE: long-standing data loss bug in initial sync of logical replication

[Zhijie Hou (Fujitsu)]

On Friday, February 28, 2025 4:28 PM Benoit Lobréau  
wrote:
> 
> It took me a while but I ran the test on my laptop with 20 runs per test. I 
> asked
> for a dedicated server and will re-run the tests if/when I have it.
> 
> count of partitions |   Head (sec) |Fix (sec) |Degradation (%)
> --
> 1000|   0,0265 |   0,028  |  5,66037735849054
> 5000|   0,091  |   0,0945 |  3,84615384615385
> 1   |   0,1795 |   0,1815 |  1,11420612813371
> 
>   Concurrent Txn |Head (sec)|Patch (sec) | Degradation in %
>   -
>   50 |   0,1797647  |   0,1920949|  6,85907744957
>   100|   0,3693029  |   0,3823425|  3,53086856344
>   500|   1,62265755 |   1,91427485   | 17,97158617972
>   1000   |   3,01388635 |   3,57678295   | 18,67676928162
>   2000   |   7,0171877  |   6,4713304|  8,43500897435
> 
> I'll try to run test2.pl later (right now it fails).
> 
> hope this helps.

Thank you for testing and sharing the data!

A nitpick with the data for the Concurrent Transaction (2000) case. The results
show that the HEAD's data appears worse than the patch data, which seems
unusual. However, I confirmed that the details in the attachment are as 
expected,
so, this seems to be a typo. (I assume you intended to use a
decimal point instead of a comma in the data like (8,43500...))

The data suggests some regression, slightly more than Shlok’s findings, but it
is still within an acceptable range for me. Since the test script builds a real
subscription for testing, the results might be affected by network and
replication factors, as Amit pointed out, we will share a new test script soon
that uses the SQL API xxx_get_changes() to test. It would be great if you could
verify the performance using the updated script as well.

Best Regards,
Hou zj

Re: Anti join confusion

[Richard Guo]

On Wed, Feb 26, 2025 at 7:09 PM Tender Wang  wrote:
> Recently, I found Greenplum implement pull-up NOT IN subquery. They have the 
> below comments in their codes:
>
> We normalize NOT subqueries using the following axioms:
> *
> * val NOT IN (subq) =>  val <> ALL (subq)
>
> Richard, do you have an impression about this?

I vaguely recall that Greenplum converts NOT IN to some form of join
in certain cases, but I can't remember the details.

Thanks
Richard

Re: Anti join confusion

[Tender Wang]

Richard Guo  于2025年3月3日周一 15:34写道：

> On Wed, Feb 26, 2025 at 7:09 PM Tender Wang  wrote:
> > Recently, I found Greenplum implement pull-up NOT IN subquery. They have
> the below comments in their codes:
> >
> > We normalize NOT subqueries using the following axioms:
> > *
> > * val NOT IN (subq) =>  val <> ALL (subq)
> >
> > Richard, do you have an impression about this?
>
> I vaguely recall that Greenplum converts NOT IN to some form of join
> in certain cases, but I can't remember the details.
>

I do some research about Greenplum planner work for the NOT IN, I think the
way they do is just like the second option you said:

* We can add support in the executor to handle the NULL semantics of
NOT IN.  This may require inventing a new join type.

1.
They add a new join type left anti semi join
2.
The executor code can handle the case when the inner side returns NULL
tuple.

I‘m not sure this transformation is only available for certain cases.  I
will continue to research.
I may provide a POC patch based on the Greenplum way.

-- 
Thanks,
Tender Wang

Selectively invalidate caches in pgoutput module

[Hayato Kuroda (Fujitsu)]

Dear hackers,

Hi, this is a fork thread from [1]. I want to propose a small optimization for
logical replication system.

Background
==

When the ALTER PUBLICATION command is executed, all entries in RelationSyncCache
will be discarded anyway. This mechanism works well but is sometimes not 
efficient.
For example, when the ALTER PUBLICATION DROP TABLE is executed,
1) the specific entry in RelationSyncCache will be removed, and then
2) all entries will be discarded twice.

This happens because the pgoutput plugin registers both RelcacheCallback
(rel_sync_cache_relation_cb) and SyscacheCallback (publication_invalidation_cb,
rel_sync_cache_publication_cb). Then, when ALTER PUBLICATION ADD/SET/DROP is 
executed,
both the relation cache of added tables and the syscache of pg_publication_rel 
and
pg_publication are invalidated.
The callback for the relation cache will remove an entry from the hash table, 
and
syscache callbacks will look up all entries and invalidate them. However, AFAICS
does not need to invalidate all of them.

I grepped source codes and found this happens since the initial version.

Currently the effect of the behavior may not be large, but [1] may affect
significantly because it propagates invalidation messages to all in-progress
decoding transactions.

Patch overview


Based on the background, the patch avoids dropping all entries in 
RelationSyncCache
when ALTER PUBLICATION is executed. It removes sys cache callbacks for 
pg_publication_rel
and pg_publication_namespace and avoids discarding entries in sys cache for 
pg_publication.

Apart from the above, this patch also ensures that relcaches of publishing 
tables
are invalidated when ALTER PUBLICATION is executed. ADD/SET/DROP already has 
this
mechanism, but ALTER PUBLICATION OWNER TO and RENAME TO do not.
Regarding RENAME TO, now we are using a common function, but it is replaced with
RenamePublication() to do invalidations.

How do you think?

[1]: 
https://www.postgresql.org/message-id/de52b282-1166-1180-45a2-8d8917ca7...@enterprisedb.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED



0001-Selectively-invalidate-cache-in-pgoutput.patch
Description: 0001-Selectively-invalidate-cache-in-pgoutput.patch

Re: Allow table AMs to define their own reloptions

[Julien Tachoires]

On Sun, Mar 02, 2025 at 09:56:41AM +0100, Julien Tachoires wrote:
> With the help of the new TAM routine 'relation_options', table access 
> methods can with this patch define their own reloptions 
> parser/validator.
> 
> These reloptions can be set via the following commands:
> 1. CREATE TABLE ... USING table_am
>WITH (option1='value1', option2='value2');
> 2. ALTER TABLE ...
>SET (option1 'value1', option2 'value2');
> 3. ALTER TABLE ... SET ACCESS METHOD table_am
>OPTIONS (option1 'value1', option2 'value2');
> 
> When changing table's access method, the settings inherited from the 
> former TAM can be dropped (if not supported by the new TAM) via: DROP 
> option, or, updated via: SET option 'value'.
> 
> Currently, tables using different TAMs than heap are able to use heap's 
> reloptions (fillfactor, toast_tuple_target, etc...). With this patch 
> applied, this is not the case anymore: if the TAM needs to have access 
> to similar settings to heap ones, they have to explicitly define them.
> 
> The 2nd patch file includes a new test module 'dummy_table_am' which 
> implements a dummy table access method utilized to exercise TAM 
> reloptions. This test module is strongly based on what we already have 
> in 'dummy_index_am'. 'dummy_table_am' provides a complete example of TAM 
> reloptions definition.
> 
> This work is directly derived from SadhuPrasad's patch here [2]. Others 
> attempts were posted here [1] and here [3].
> 
> [1] 
> https://www.postgresql.org/message-id/flat/429fb58fa3218221bb17c7bf9e70e1aa6cfc6b5d.camel%40j-davis.com
> [2] 
> https://www.postgresql.org/message-id/flat/caff0-cg4kzhdtyhmsonwixnzj16gwzpduxan8yf7pddub+g...@mail.gmail.com
> [3] 
> https://www.postgresql.org/message-id/flat/AMUA1wBBBxfc3tKRLLdU64rb.1.1683276279979.Hmail.wuhao%40hashdata.cn

Please find a new version including minor fixes: 'TAM' terms are
replaced by 'table AM'

-- 
Julien Tachoires
>From 4b20842ae509f6c330c48e6792fd4c966e3f Mon Sep 17 00:00:00 2001
From: Julien Tachoires 
Date: Sat, 1 Mar 2025 17:59:49 +0100
Subject: [PATCH 1/2] Allow table AMs to define their own reloptions

With the help of the new routine 'relation_options', table access
methods can now define their own reloptions.

These options can be set via the following commands:
1. CREATE TABLE ... USING table_am
   WITH (option1='value1', option2='value2');
2. ALTER TABLE ...
   SET (option1 'value1', option2 'value2');
3. ALTER TABLE ... SET ACCESS METHOD table_am
   OPTIONS (option1 'value1', option2 'value2');

When changing table's access method, the settings from the former
table AM can be dropped (if not supported by the new table AM) via:
DROP option, or, updated via: SET option 'value'.

Before this commit, tables using different table AMs than heap were
able to use heap's reloptions (fillfactor, toast_tuple_target,
etc...). Now, this is not the case anymore: if the table AM needs
to have access to settings similar to heap ones, they must
explicitly define them.

This work is directly derived from SadhuPrasad's patch named:
v4-0001-PATCH-V4-Per-table-storage-parameters-for-TableAM.patch
---
 doc/src/sgml/ref/alter_table.sgml|  13 +-
 doc/src/sgml/ref/create_table.sgml   |   3 +-
 src/backend/access/common/reloptions.c   |  66 -
 src/backend/access/heap/heapam_handler.c |   2 +
 src/backend/commands/foreigncmds.c   |   2 +-
 src/backend/commands/tablecmds.c | 180 ---
 src/backend/parser/gram.y|   9 ++
 src/backend/postmaster/autovacuum.c  |  18 ++-
 src/backend/utils/cache/relcache.c   |  11 +-
 src/include/access/reloptions.h  |   6 +-
 src/include/access/tableam.h |  10 ++
 src/include/commands/defrem.h|   1 +
 12 files changed, 286 insertions(+), 35 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8e56b8e59b0..e38200e20d2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -76,7 +76,7 @@ ALTER TABLE [ IF EXISTS ] name
 CLUSTER ON index_name
 SET WITHOUT CLUSTER
 SET WITHOUT OIDS
-SET ACCESS METHOD { new_access_method | DEFAULT }
+SET ACCESS METHOD { new_access_method | DEFAULT } [ OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ] ) ]
 SET TABLESPACE new_tablespace
 SET { LOGGED | UNLOGGED }
 SET ( storage_parameter [= value] [, ... ] )
@@ -734,7 +734,7 @@ WITH ( MODULUS numeric_literal, REM

 

-SET ACCESS METHOD
+SET ACCESS METHOD { new_access_method | DEFAULT } [ OPTIONS ( [ ADD | SET | DROP ] option ['value'] [, ... ] ) ]
 
  
   This form changes the access method of the table by rewriting it
@@ -752,6 +752,15 @@ WITH ( MODULUS numeric_literal, REM
   causing future partitions to default to
   default_table_access_method.
  
+ 
+  Specifying OPTIONS allows to change options for
+  the table when changing t

Re: Statistics Import and Export

[Magnus Hagander]

On Sat, Mar 1, 2025 at 9:48 PM Jeff Davis  wrote:

> On Sat, 2025-03-01 at 13:52 -0500, Greg Sabino Mullane wrote:
> > > Can you expand on some of those cases?
> >
> > Certainly. I think one of the problems is that because this patch is
> > solving a pg_upgrade issue, the focus is on the "dump and restore"
> > scenarios. But pg_dump is used for much more than that, especially
> > "dump and examine".
>
> Thank you for going through these examples.
>
> > I just don't think it should be enabled by default for everything
> > using pg_dump. For the record, I would not strongly object to having
> > stats on by default for binary dumps, although I would prefer them
> > off.
>
> I am open to that idea, I just want to get it right, because probably
> whatever the default is in 18 will stay that way.
>
> Also, we will need to think through the set of pg_dump options again. A
> lot of our tools seem to assume that "if it's the default, we don't
> need a way to ask for it explicitly", which makes it a lot harder to
> ever change the default and keep a coherent set of options.
>

That's a good point in general, and definitely something we should think
through, independently of his patch.


> > So why not just expect people to modify their programs to use --no-
> > statistics for cases like this? That's certainly an option, but it's
> > going to break a lot of existing things, and create branching code:
>
> I suggest that we wait a bit to see what additional feedback we get
> early in beta.
>

I definitely thing it should be on by default.

FWIW, I've seen many cases of people using automated tools to verify the
*schema* between two databases. I'd say that's quite common. But they use
pg_dump -s, which I believe is not affected by this one.

I don't think I've ever come across an automated tool to verify the
contents of an entire database this way. That doesn't mean it's not out
there of course, just that it's not so common. The cases I've seen pg_dump
used to verify the contents that's always been in combination with a myriad
of other switches such as include/exclude of specific tables etc, and
adding just one more switch to those seems like a small price to pay for
having the default behaviour be a big improvement for the majority of
usecases.


> Also, anything trained to parse pg_dump output will have to learn
> > about the new SELECT pg_restore_ calls with their multi-line formats
> > (not 100% sure we don't have that anywhere, as things like "SELECT
> > setval" and "SELECT set_config" are single line, but there may be
> > existing things)
>

That's going to be true every time we add something to pg_dump. And for
that matter, anything new to *postgresql*, since surely we'd want pg_dump
to dump objects by default. Any tool that parses the pg_dump output
directly will always have to carefully analyze each new version. And
probably shouldn't be using the plaintext format in the first place - and
if using pg_restore it comes out as it's own type of object, making it easy
to exclude at that level.

--
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Statistics Import and Export

[Corey Huinker]

>
> Also, we will need to think through the set of pg_dump options again. A
>> lot of our tools seem to assume that "if it's the default, we don't
>> need a way to ask for it explicitly", which makes it a lot harder to
>> ever change the default and keep a coherent set of options.
>>
>
> That's a good point in general, and definitely something we should think
> through, independently of his patch.
>

I agree. There was a --with-statistics option in earlier patchsets, which
was effectively a no-op because statistics are the default, and it was
removed when its existence was questioned. I mention this only to say that
consensus for those options will have to be built.



> FWIW, I've seen many cases of people using automated tools to verify the
> *schema* between two databases. I'd say that's quite common. But they use
> pg_dump -s, which I believe is not affected by this one.
>

Correct, -s behaves as before, as does --data-only. Schema, data, and
statistics are independent, each has their own -only flag, each each has
their own --no- flag.

If you were using --no-schema to mean data-only, or --no-data to mean
schema-only, then you'll have to add --no-statistics to that call, but I'd
argue that they already had a better option of getting what they wanted.

If you thought you saw major changes in the patchsets around those flags,
you weren't imagining it. There was a lot of internal logic that worked on
the assumptions like "If schema_only is false then we must want data" but
that's no longer strictly true, so we resolved all the user flags to
dumpSchema/dumpData/dumpStatistics at the very start, and now the internal
logic work is based on those affirmative flags rather than the bankshot
absence-of-the-opposite logic that was there before.

>

Re: Optimization for lower(), upper(), casefold() functions.

[Alexander Borisov]

19.02.2025 01:56, Jeff Davis пишет:

On Wed, 2025-02-19 at 01:54 +0300, Alexander Borisov wrote:

In proposing the patch for v3, I struck a balance between improving
performance and reducing binary size, without sacrificing code
clarity.


Fair enough. I will continue reviewing v3.


Did you have a time for review this?

I'd like to continue improving Unicode in Postgres, as I previously
wrote, next in my plans are Normalization forms, and more.
But now I am blocked by this patch.


--
Alexander Borisov