Re: Psql meta-command conninfo+

[Pavel Luzanov]

Hi, Maiquel!

The patch v10 build ends with a warning:
$ make -j --silent
describe.c:911:1: warning: no previous prototype for 
‘listConnectionInformation’ [-Wmissing-prototypes]
  911 | listConnectionInformation()
  | ^

About terms.

postgres@postgres(17.0)=# \x \conninfo+
Expanded display is on.
Current Connection Information
-[ RECORD 1 ]--+-
Database   | postgres
Authenticated User | postgres
System User|
Current User   | postgres
Session User   | postgres
*Session PID | 951112 *Server Version | 17devel
Server Address |
Server Port| 5401
Client Address |
Client Port|
Socket Directory   | /tmp
Host   |

It looks like "Session PID" is a new term for the server process identifier.
How about changing the name to "Backend PID" (from pg_backend_pid) or even PID 
(from pg_stat_activity)?

On 08.02.2024 17:58, Maiquel Grassi wrote:
> 1. > + if (db == NULL) > + printf(_("You are currently not connected to 
a database.\n")); > > This check is performed for \conninfo, but not 
for \conninfo+.
1. The connection check for the case of \conninfo+ is handled by 
"describe.c" itself since it deals with queries. I might be mistaken, 
but I believe that by using "printQuery()" via "describe.c", this is 
already ensured, and there is no need to evaluate the connection status.


I found that \conninfo and \conninfo+ act differently when the connection is 
broken.
I used pg_terminate_backend function from another session to terminate an open 
psql session.
After that, \conninfo does not see the connection break (surprisingly!), and 
\conninfo+ returns an error:

postgres@postgres(17.0)=# \conninfo+
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

postgres@postgres(17.0)=# \conninfo
You are connected to database "postgres" as user "postgres" via socket in "/tmp" at port 
"5401".

Another surprise is that this check:if (db == NULL) did not work in both cases.

--
Pavel Luzanov
Postgres Professional:https://postgrespro.com

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 9:39 PM Tom Lane  wrote:

> Nathan Bossart  writes:
> > On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:
> >> I'd put these static inlines into common/int.h. I don't think this is
> common
> >> enough to warrant being in c.h. Probably also doesn't hurt to have a
> not quite
> >> as generic name as INT_CMP, I'd not be too surprised if that's defined
> in some
> >> library.
> >>
> >> I think it's worth following int.h's pattern of including
> [s]igned/[u]nsigned
> >> in the name, an efficient implementation for signed might not be the
> same as
> >> for unsigned. And if we use static inlines, we need to do so for correct
> >> semantics anyway.
>
> > Seems reasonable to me.
>
> +1 here also.
>

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the arithmetic,
so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

The types "int" and "size_t" are treated as s32 and u32 respectively since
that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

I also noted that in many situations size_t values are treated as "int" so
there is an overflow risk here, even if small. For example, the result of
"list_length" is assigned to an integer. I do not think this is an
immediate concern, but just wanted to mention it.

Best wishes,
Mats Kindahl

>
> regards, tom lane
>
From 6980199cefde28b4ccf5f0fc3d89e001e8b0c4ef Mon Sep 17 00:00:00 2001
From: Mats Kindahl 
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: Add integer comparison functions for qsort()

Introduce functions pg_cmp_xyz() that will standardize integer comparison
for implementing comparison function for qsort(). The functions will
returns negative, 0, or positive integer without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.

For integer sizes less than sizeof(int) we can use normal subtraction,
which is faster, but for integers with size greater than or equal to
sizeof(int) we need to handle this differently.
---
 contrib/hstore/hstore_gist.c  |  2 +-
 contrib/intarray/_intbig_gist.c   |  2 +-
 .../pg_stat_statements/pg_stat_statements.c   |  7 +---
 contrib/pg_trgm/trgm_op.c |  7 +---
 contrib/seg/seg.c |  7 +---
 src/backend/access/nbtree/nbtinsert.c |  8 +---
 src/backend/access/nbtree/nbtpage.c   | 10 ++---
 src/backend/access/nbtree/nbtsplitloc.c   |  8 +---
 src/backend/access/spgist/spgdoinsert.c   |  5 +--
 src/backend/access/spgist/spgtextproc.c   |  3 +-
 src/backend/backup/basebackup_incremental.c   |  8 +---
 src/backend/backup/walsummary.c   |  7 +---
 src/backend/catalog/heap.c|  8 +---
 src/backend/nodes/list.c  | 13 ++
 src/backend/nodes/tidbitmap.c |  7 +---
 src/backend/parser/parse_agg.c|  3 +-
 src/backend/postmaster/autovacuum.c   |  6 +--
 .../replication/logical/reorderbuffer.c   |  7 +---
 src/backend/replication/syncrep.c |  8 +---
 src/backend/utils/adt/oid.c   |  7 +---
 src/backend/utils/adt/tsgistidx.c | 10 ++---
 src/backend/utils/adt/tsquery_gist.c  |  6 +--
 src/backend/utils/adt/tsvector.c  |  5 +--
 src/backend/utils/adt/tsvector_op.c   |  5 +--
 src/backend/utils/adt/xid.c   |  7 +---
 src/backend/utils/cache/relcache.c|  3 +-
 src/backend/utils/cache/syscache.c|  5 +--
 src/backend/utils/cache/typcache.c|  8 +---
 src/backend/utils/resowner/resowner.c | 10 +
 src/bin/pg_dump/pg_dump_sort.c|  7 +---
 src/bin/pg_upgrade/function.c | 13 +++---
 src/bin/pg_walsummary/pg_walsummary.c |  8 +---
 src/bin/psql/crosstabview.c   |  3 +-
 src/include/access/gin_private.h  |  8 +---
 src/include/common/int.h  | 41 +++
 35 files changed, 112 insertions(+), 160 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..e125b76290 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -356,7 +356,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git 

Re: Introduce XID age and inactive timeout based replication slot invalidation

[Bertrand Drouvot]

Hi,

On Wed, Feb 07, 2024 at 12:22:07AM +0530, Bharath Rupireddy wrote:
> On Mon, Feb 5, 2024 at 3:15 PM Bertrand Drouvot
>  wrote:
> >
> > I'm not sure I like the fact that "invalidations" and "conflicts" are merged
> > into a single field. I'd vote to keep conflict_reason as it is and add a new
> > invalidation_reason (and put "conflict" as value when it is the case). The 
> > reason
> > is that I think they are 2 different concepts (could be linked though) and 
> > that
> > it would be easier to check for conflicts (means conflict_reason is not 
> > NULL).
> 
> So, do you want conflict_reason for only logical slots, and a separate
> column for invalidation_reason for both logical and physical slots?

Yes, with "conflict" as value in case of conflicts (and one would need to refer
to the conflict_reason reason to see the reason).

> Is there any strong reason to have two properties "conflict" and
> "invalidated" for slots?

I think "conflict" is an important topic and does contain several reasons. The
slot "first" conflict and then leads to slot "invalidation". 

> They both are the same internally, so why
> confuse the users?

I don't think that would confuse the users, I do think that would be easier to
check for conflicting slots.

I did not look closely at the code, just played a bit with the patch and was 
able
to produce something like:

postgres=# select 
slot_name,slot_type,active,active_pid,wal_status,invalidation_reason from 
pg_replication_slots;
  slot_name  | slot_type | active | active_pid | wal_status | 
invalidation_reason
-+---++++-
 rep1| physical  | f  || reserved   |
 master_slot | physical  | t  |1482441 | unreserved | wal_removed
(2 rows)

does that make sense to have an "active/working" slot "ivalidated"?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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

[Sutou Kouhei]

Hi,

In 
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on 
Fri, 9 Feb 2024 13:21:34 +0900,
  Michael Paquier  wrote:

>> - Revisit what we have here, looking at more profiles to see how HEAD
>> an v13 compare.  It looks like we are on a good path, but let's tackle
>> things one step at a time.
> 
> And attached is a v14 that's rebased on HEAD.

Thanks!

> A next step I think we could take is to split the binary-only and the
> text/csv-only data in each cstate into their own structure to make the
> structure, with an opaque pointer that custom formats could use, but a
> lot of fields are shared as well.

It'll make COPY code base cleaner but it may decrease
performance. How about just adding an opaque pointer to each
cstate as the next step and then try the split?

My suggestion:
1. Introduce Copy{To,From}Routine
   (We can do it based on the v14 patch.)
2. Add an opaque pointer to Copy{To,From}Routine
   (This must not have performance impact.)
3.a. Split format specific data to the opaque space
3.b. Add support for registering custom format handler by
 creating a function
4. ...

>This patch is already complicated
> enough IMO, so I'm OK to leave it out for the moment, and focus on
> making this infra pluggable as a next step.

I agree with you.

> Are there any comments about this v14?  Sutou-san?

Here are my comments:


+   /* Set read attribute callback */
+   if (cstate->opts.csv_mode)
+   cstate->copy_read_attributes = CopyReadAttributesCSV;
+   else
+   cstate->copy_read_attributes = CopyReadAttributesText;

I think that we should not use this approach for
performance. We need to use "static inline" and constant
argument instead something like the attached
remove-copy-read-attributes.diff.

We have similar codes for
CopyReadLine()/CopyReadLineText(). The attached
remove-copy-read-attributes-and-optimize-copy-read-line.diff
also applies the same optimization to
CopyReadLine()/CopyReadLineText().

I hope that this improved performance of COPY FROM.

+/*
+ * Routines assigned to each format.
++

Garbage "+"

+ * CSV and text share the same implementation, at the exception of the
+ * copy_read_attributes callback.
+ */


+/*
+ * CopyToTextOneRow
+ *
+ * Process one row for text/CSV format.
+ */
+static void
+CopyToTextOneRow(CopyToState cstate,
+TupleTableSlot *slot)
+{
...
+   if (cstate->opts.csv_mode)
+   CopyAttributeOutCSV(cstate, string,
+   
cstate->opts.force_quote_flags[attnum - 1]);
+   else
+   CopyAttributeOutText(cstate, string);
...

How about use "static inline" and constant argument approach
here too?

static inline void
CopyToTextBasedOneRow(CopyToState cstate,
  TupleTableSlot *slot,
  bool csv_mode)
{
...
if (cstate->opts.csv_mode)
CopyAttributeOutCSV(cstate, string,

cstate->opts.force_quote_flags[attnum - 1]);
else
CopyAttributeOutText(cstate, string);
...
}

static void
CopyToTextOneRow(CopyToState cstate,
 TupleTableSlot *slot,
 bool csv_mode)
{
CopyToTextBasedOneRow(cstate, slot, false);
}

static void
CopyToCSVOneRow(CopyToState cstate,
TupleTableSlot *slot,
bool csv_mode)
{
CopyToTextBasedOneRow(cstate, slot, true);
}

static const CopyToRoutine CopyCSVRoutineText = {
...
.CopyToOneRow = CopyToCSVOneRow,
...
};


Thanks,
-- 
kou
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a90b7189b5..6e244fb443 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -158,12 +158,6 @@ CopyFromTextStart(CopyFromState cstate, TupleDesc tupDesc)
 	attr_count = list_length(cstate->attnumlist);
 	cstate->max_fields = attr_count;
 	cstate->raw_fields = (char **) palloc(attr_count * sizeof(char *));
-
-	/* Set read attribute callback */
-	if (cstate->opts.csv_mode)
-		cstate->copy_read_attributes = CopyReadAttributesCSV;
-	else
-		cstate->copy_read_attributes = CopyReadAttributesText;
 }
 
 /*
@@ -221,9 +215,8 @@ CopyFromBinaryEnd(CopyFromState cstate)
 
 /*
  * Routines assigned to each format.
-+
  * CSV and text share the same implementation, at the exception of the
- * copy_read_attributes callback.
+ * CopyFromOneRow callback.
  */
 static const CopyFromRoutine CopyFromRoutineText = {
 	.CopyFromInFunc = CopyFromTextInFunc,
@@ -235,7 +228,7 @@ static const CopyFromRoutine CopyFromRoutineText = {
 static 

Re: Psql meta-command conninfo+

[Jim Jones]

Hi Nathan

On 09.02.24 03:41, Nathan Bossart wrote:

> Separately, does
> the server version really belong here?  I'm not sure I would consider that
> to be connection information.
>
I tend to agree with you. The info there wouldn't hurt, but perhaps the
client version would be a better fit.

-- 
Jim

Re: Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN

[Peter Eisentraut]

On 08.02.24 07:04, Ashutosh Bapat wrote:

The patch looks ok.

+seqstmt->sequence->relpersistence = cxt->rel ?
cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
+

This condition looks consistent with the other places in the code
around line 435, 498.


Ah good, that pattern already existed.


But I was worried that cxt->rel may not get
latest relpersistence if the ALTER TABLE changes persistence as well.
Added a test (0002) which shows that ctx->rel has up-to-date
relpersistence. Also added a few other tests. Feel free to
include/reject them while committing.


Yes, this additional coverage seems good.  Committed with your additions.

RE: Synchronizing slots from primary to standby

[Zhijie Hou (Fujitsu)]

On Friday, February 9, 2024 2:44 PM Masahiko Sawada  
wrote:
> 
> On Thu, Feb 8, 2024 at 8:01 PM shveta malik  wrote:
> >
> > On Thu, Feb 8, 2024 at 12:08 PM Peter Smith 
> wrote:
> > >
> > > Here are some review comments for patch v80_2-0001.
> >
> > Thanks for the feedback Peter. Addressed the comments in v81.
> > Attached patch001 for early feedback. Rest of the patches need
> > rebasing and thus will post those later.
> >
> > It also addresses comments by Amit in [1].
> 
> Thank you for updating the patch! Here are random comments:

Thanks for the comments!

> 
> ---
> +
> +/*
> + * Register the callback function to clean up the shared memory of
> slot
> + * synchronization.
> + */
> +SlotSyncInitialize();
> 
> I think it would have a wider impact than expected. IIUC this callback is 
> needed
> only for processes who calls synchronize_slots(). Why do we want all processes
> to register this callback?

I think the current style is similar to the ReplicationSlotInitialize() above 
it. For backend,
both of them can only be used when user calls slot SQL functions. So, I think 
it could be fine to
register it at the general place which can also avoid registering the same 
again for the later
slotsync worker patch.

Another alternative is to register the callback when calling slotsync functions
and unregister it after the function call. And register the callback in
slotsyncworkmain() for the slotsync worker patch, although this may adds a few
more codes.

Best Regards,
Hou zj

Re: Documentation to upgrade logical replication cluster

[Peter Smith]

Here are some review comments for patch v7-0001.

==
doc/src/sgml/glossary.sgml

1.
+  
+   Logical replication cluster
+   
+
+ A set of publisher and subscriber instance with publisher instance
+ replicating changes to the subscriber instance.
+
+   
+  

1a.
/instance with/instances with/

~~~

1b.
The description then made me want to look up the glossary definition
of a "publisher instance" and "subscriber instance", but then I was
quite surprised that even "Publisher" and "Subscriber" terms are not
described in the glossary. Should this patch add those, or should we
start another thread for adding them?

==
doc/src/sgml/logical-replication.sgml

2.
+  
+   Migration of logical replication clusters is possible only when all the
+   members of the old logical replication clusters are version 17.0 or later.
+  

Here is where "logical replication clusters" is mentioned. Shouldn't
this first reference be linked to that new to the glossary entry --
e.g. logical replication clusters

~~~

3.
+   
+Following are the prerequisites for pg_upgrade
+to be able to upgrade the logical slots. If these are not met an error
+will be reported.
+   

SUGGESTION
The following prerequisites are required for ...

~~~

4.
+ 
+  All slots on the old cluster must be usable, i.e., there are no slots
+  whose
+  pg_replication_slots.conflict_reason
+  is not NULL.
+ 

The double-negative is too tricky "no slots whose ... not NULL", needs
rewording. Maybe it is better to instead use an example as the next
bullet point does.

~~~

5.
+
+   
+Following are the prerequisites for
pg_upgrade to
+be able to upgrade the subscriptions. If these are not met an error
+will be reported.
+   

SUGGESTION
The following prerequisites are required for ...

==
doc/src/sgml/ref/pgupgrade.sgml

6.
+   
+
+ The steps to upgrade logical replication clusters are not covered here;
+ refer to  for details.
+
+   

Maybe here too there should be a link to the glossary term "logical
replication clusters".

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Race condition in FetchTableStates() breaks synchronization of subscription tables

[vignesh C]

On Thu, 8 Feb 2024 at 23:30, Alexander Lakhin  wrote:
>
> 08.02.2024 12:25, vignesh C wrote:
> > Yes, the wakeup call sent by the "CREATE SUBSCRIPTION" command was
> > getting missed in this case. The wakeup call can be sent during
> > subscription creation/modification and when the apply worker exits.
> > WaitForReplicationWorkerAttach should not reset the latch here as it
> > will end up delaying the apply worker to get started after 180 seconds
> > timeout(DEFAULT_NAPTIME_PER_CYCLE). The attached patch does not reset
> > the latch and lets ApplyLauncherMain to reset the latch and checks if
> > any new worker or missing worker needs to be started.
>
> Thank you for the updated patch!
> I ran all the subscription tests in a loop (with the sleeps added as
> before) and observed no failures and 180+ seconds duration.

Thanks, I have created the following Commitfest entry for this:
https://commitfest.postgresql.org/47/4816/

Regards,
Vignesh

Re: Synchronizing slots from primary to standby

[Masahiko Sawada]

On Thu, Feb 8, 2024 at 8:01 PM shveta malik  wrote:
>
> On Thu, Feb 8, 2024 at 12:08 PM Peter Smith  wrote:
> >
> > Here are some review comments for patch v80_2-0001.
>
> Thanks for the feedback Peter. Addressed the comments in v81.
> Attached patch001 for early feedback. Rest of the patches need
> rebasing and thus will post those later.
>
> It also addresses comments by Amit in [1].

Thank you for updating the patch! Here are random comments:

---
+ereport(ERROR,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("cannot use replication slot
\"%s\" for logical"
+   " decoding",
NameStr(slot->data.name)),
+errdetail("This slot is being synced
from the primary server."\),
+errhint("Specify another replication slot."));
+

I think it's better to use "synchronized" instead of "synced" for
consistency with other places.

---
We can create a temporary failover slot on the primary, but such slot
is not synchronized. Do we want to disallow creating it?

---
+
+/*
+ * Register the callback function to clean up the shared memory of slot
+ * synchronization.
+ */
+SlotSyncInitialize();

I think it would have a wider impact than expected. IIUC this callback
is needed only for processes who calls synchronize_slots(). Why do we
want all processes to register this callback?

---
+if (!valid)
+ereport(ERROR,
+errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("bad configuration for slot
synchronization"),
+/* translator: second %s is a GUC variable name */
+errdetail("The primary server slot
\"%s\" specified by \"%s\" i\s not valid.",
+  PrimarySlotName,
"primary_slot_name"));
+

I think that the detail message is not appropriate since the
primary_slot_name could actually be a valid name. I think we can
rephrase it to something like "The replication slot %s specified by %s
does not exist on the primary server".

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Small fix on query_id_enabled

[Yugo NAGATA]

Hi,

I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON.

 /* True when compute_query_id is ON, or AUTO and a module requests them */
 bool   query_id_enabled = false;

Should we fix this as following (just fixing the place of a comma) ?

/* True when compute_query_id is ON or AUTO, and a module requests them */

Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.

Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled,  instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?

I attached a patch for above fixes. 

Although renaming might  not be acceptable since changing global variables
may affect third party tools, I think the comment should be fixed at least.

IMHO, it seems better to make this variable static not to be accessed directly.
However, I left it as is because this is used in a static inline function.

Regards,
Yugo Nagata

-- 
Yugo NAGATA 
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb5..e4fbcf0d9f 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -42,8 +42,8 @@
 /* GUC parameters */
 int			compute_query_id = COMPUTE_QUERY_ID_AUTO;
 
-/* True when compute_query_id is ON, or AUTO and a module requests them */
-bool		query_id_enabled = false;
+/* True when compute_query_id is ON or AUTO, and a module requests them */
+bool		query_id_required = false;
 
 static void AppendJumble(JumbleState *jstate,
 		 const unsigned char *item, Size size);
@@ -145,7 +145,7 @@ void
 EnableQueryId(void)
 {
 	if (compute_query_id != COMPUTE_QUERY_ID_OFF)
-		query_id_enabled = true;
+		query_id_required = true;
 }
 
 /*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a066800a1c..4bcaf6d71c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -532,7 +532,7 @@ typedef struct
 	pg_time_t	first_syslogger_file_time;
 	bool		redirection_done;
 	bool		IsBinaryUpgrade;
-	bool		query_id_enabled;
+	bool		query_id_required;
 	int			max_safe_fds;
 	int			MaxBackends;
 #ifdef WIN32
@@ -6103,7 +6103,7 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
 
 	param->redirection_done = redirection_done;
 	param->IsBinaryUpgrade = IsBinaryUpgrade;
-	param->query_id_enabled = query_id_enabled;
+	param->query_id_required = query_id_required;
 	param->max_safe_fds = max_safe_fds;
 
 	param->MaxBackends = MaxBackends;
@@ -6348,7 +6348,7 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
 
 	redirection_done = param->redirection_done;
 	IsBinaryUpgrade = param->IsBinaryUpgrade;
-	query_id_enabled = param->query_id_enabled;
+	query_id_required = param->query_id_required;
 	max_safe_fds = param->max_safe_fds;
 
 	MaxBackends = param->MaxBackends;
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index f1c55c8067..3b2e1f8018 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -67,7 +67,7 @@ extern const char *CleanQuerytext(const char *query, int *location, int *len);
 extern JumbleState *JumbleQuery(Query *query);
 extern void EnableQueryId(void);
 
-extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_required;
 
 /*
  * Returns whether query identifier computation has been enabled, either
@@ -80,7 +80,7 @@ IsQueryIdEnabled(void)
 		return false;
 	if (compute_query_id == COMPUTE_QUERY_ID_ON)
 		return true;
-	return query_id_enabled;
+	return query_id_required;
 }
 
 #endif			/* QUERYJUMBLE_H */

Re: speed up a logical replica setup

[vignesh C]

On Wed, 7 Feb 2024 at 10:24, Euler Taveira  wrote:
>
> On Fri, Feb 2, 2024, at 6:41 AM, Hayato Kuroda (Fujitsu) wrote:
>
> Thanks for updating the patch!

Thanks for the updated patch, few comments:
Few comments:
1) Cleanup function handler flag should be reset, i.e.
dbinfo->made_replslot = false; should be there else there will be an
error during  drop replication slot cleanup in error flow:
+static void
+drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
char *slot_name)
+{
+   PQExpBuffer str = createPQExpBuffer();
+   PGresult   *res;
+
+   Assert(conn != NULL);
+
+   pg_log_info("dropping the replication slot \"%s\" on database
\"%s\"", slot_name, dbinfo->dbname);
+
+   appendPQExpBuffer(str, "SELECT
pg_drop_replication_slot('%s')", slot_name);
+
+   pg_log_debug("command is: %s", str->data);

2) Cleanup function handler flag should be reset, i.e.
dbinfo->made_publication = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove publication if it couldn't finish all steps.
+ */
+static void
+drop_publication(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+   PQExpBuffer str = createPQExpBuffer();
+   PGresult   *res;
+
+   Assert(conn != NULL);
+
+   pg_log_info("dropping publication \"%s\" on database \"%s\"",
dbinfo->pubname, dbinfo->dbname);
+
+   appendPQExpBuffer(str, "DROP PUBLICATION %s", dbinfo->pubname);
+
+   pg_log_debug("command is: %s", str->data);

3) Cleanup function handler flag should be reset, i.e.
dbinfo->made_subscription = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove subscription if it couldn't finish all steps.
+ */
+static void
+drop_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+   PQExpBuffer str = createPQExpBuffer();
+   PGresult   *res;
+
+   Assert(conn != NULL);
+
+   pg_log_info("dropping subscription \"%s\" on database \"%s\"",
dbinfo->subname, dbinfo->dbname);
+
+   appendPQExpBuffer(str, "DROP SUBSCRIPTION %s", dbinfo->subname);
+
+   pg_log_debug("command is: %s", str->data);

4) I was not sure if drop_publication is required here, as we will not
create any publication in subscriber node:
+   if (dbinfo[i].made_subscription)
+   {
+   conn = connect_database(dbinfo[i].subconninfo);
+   if (conn != NULL)
+   {
+   drop_subscription(conn, [i]);
+   if (dbinfo[i].made_publication)
+   drop_publication(conn, [i]);
+   disconnect_database(conn);
+   }
+   }

5) The connection should be disconnected in case of error case:
+   /* secure search_path */
+   res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
+   if (PQresultStatus(res) != PGRES_TUPLES_OK)
+   {
+   pg_log_error("could not clear search_path: %s",
PQresultErrorMessage(res));
+   return NULL;
+   }
+   PQclear(res);

6) There should be a line break before postgres_fe inclusion, to keep
it consistent:
+ *-
+ */
+#include "postgres_fe.h"
+
+#include 

7) These includes are not required:
7.a) #include 
7.b) #include 
7.c) #include 
7.d) #include "access/xlogdefs.h"
7.e) #include "catalog/pg_control.h"
7.f) #include "common/file_utils.h"
7.g) #include "utils/pidfile.h"

+ * src/bin/pg_basebackup/pg_createsubscriber.c
+ *
+ *-
+ */
+#include "postgres_fe.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "access/xlogdefs.h"
+#include "catalog/pg_authid_d.h"
+#include "catalog/pg_control.h"
+#include "common/connect.h"
+#include "common/controldata_utils.h"
+#include "common/file_perm.h"
+#include "common/file_utils.h"
+#include "common/logging.h"
+#include "common/restricted_token.h"
+#include "fe_utils/recovery_gen.h"
+#include "fe_utils/simple_list.h"
+#include "getopt_long.h"
+#include "utils/pidfile.h"

8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it
required or kept for future purpose:
+enum WaitPMResult
+{
+   POSTMASTER_READY,
+   POSTMASTER_STANDBY,
+   POSTMASTER_STILL_STARTING,
+   POSTMASTER_FAILED
+};

9) pg_createsubscriber should be kept after pg_basebackup to maintain
the consistent order:
diff --git a/src/bin/pg_basebackup/.gitignore b/src/bin/pg_basebackup/.gitignore
index 26048bdbd8..b3a6f5a2fe 100644
--- a/src/bin/pg_basebackup/.gitignore
+++ b/src/bin/pg_basebackup/.gitignore
@@ -1,5 +1,6 @@
 /pg_basebackup
 /pg_receivewal
 /pg_recvlogical
+/pg_createsubscriber

10) dry-run help message is not very clear, how about something
similar to pg_upgrade's message like "check clusters only, don't
change 

Re: Synchronizing slots from primary to standby

[Amit Kapila]

On Fri, Feb 9, 2024 at 9:57 AM Peter Smith  wrote:
>
> Here are some review comments for patch v81-0001.
>
> ==
>
> 1. GENERAL - ReplicationSlotInvalidationCause enum.
>
> I was thinking that the ReplicationSlotInvalidationCause should
> explicitly set RS_INVAL_NONE = 0 (it's zero anyway, but making it
> explicit with a comment /* Must be zero. */. will stop it from being
> changed in the future).
>
> --
> /*
>  * Slots can be invalidated, e.g. due to max_slot_wal_keep_size. If so, the
>  * 'invalidated' field is set to a value other than _NONE.
>  */
> typedef enum ReplicationSlotInvalidationCause
> {
>   RS_INVAL_NONE = 0, /* Must be zero. */
>   ...
> } ReplicationSlotInvalidationCause;
> --
>
> The reason to do this is because many places in the patch check for
> RS_INVAL_NONE, but if RS_INVAL_NONE == 0 is assured, all those code
> fragments can be simplified and IMO also become more readable.
>
> e.g. update_local_synced_slot()
>
> BEFORE
> Assert(slot->data.invalidated == RS_INVAL_NONE);
>
> AFTER
> Assert(!slot->data.invalidated);
>

I find the current code style more intuitive.

>
> 5. synchronize_slots
>
> + /*
> + * The primary_slot_name is not set yet or WALs not received yet.
> + * Synchronization is not possible if the walreceiver is not started.
> + */
> + latestWalEnd = GetWalRcvLatestWalEnd();
> + SpinLockAcquire(>mutex);
> + if ((WalRcv->slotname[0] == '\0') ||
> + XLogRecPtrIsInvalid(latestWalEnd))
> + {
> + SpinLockRelease(>mutex);
> + return;
> + }
> + SpinLockRelease(>mutex);
>
> The comment talks about the GUC "primary_slot_name", but the code is
> checking the WalRcv's slotname. It may be the same, but the difference
> is confusing.
>

Yeah, in this case, it would be the same because we don't allow slot
sync worker unless primary_slot_name is configured in which case
WalRcv->slotname refers to primary_slot_name. However, I think it is
better to explain here why slot synchronization is not possible or
doesn't make sense till walreceiver starts streaming and in which
case, won't it be sufficient to just check latestWalEnd?

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Add native windows on arm64 support

[Michael Paquier]

On Tue, Feb 06, 2024 at 07:01:49AM -0500, Dave Cramer wrote:
> Thanks, this patch works and
> testing with meson passes.

Only with the version posted at [1]?  Interesting, that's the same
contents as v8 posted upthread, minus src/tools/ because we don't need
to care about them anymore.

Andrew, what's happening on the test side?  It does not seem you've
mentioned any details about what is going wrong, or I have just missed
them.

> I'll try the buildfarm next.

[1]: 
https://www.postgresql.org/message-id/ea42654a-3dc4-98b0-335b-56b7ec5e5...@dunslane.net
--
Michael


signature.asc
Description: PGP signature

Re: GUC-ify walsender MAX_SEND_SIZE constant

[Michael Paquier]

On Thu, Feb 08, 2024 at 02:42:00PM +0330, Majid Garoosi wrote:
> Thank you very much for your review.

Something to be aware of, but the community lists use bottom-posting
for replies because it is easier to follow the logic of a thread this
way.  See here:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting

> I generally agree with your suggestions, so just applied them.
> You can find the new patch in the attached file.

Thanks for the patch, that looks rather fine.  I have spent some time
polishing the docs, adding a mention that increasing the value can
show benefits depending on what you do.  How does the attached look to
you?
--
Michael
From 2bacd4e4aaff53d26d8017e905e1dd02a67e93ff Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 9 Feb 2024 14:18:31 +0900
Subject: [PATCH v3] Introduce wal_sender_max_send_size as a GUC

Blah, blah.

Author: Majid Garoosi
Discussion: https://postgr.es/m/
---
 src/include/replication/walsender.h   |  1 +
 src/include/utils/guc_hooks.h |  1 +
 src/backend/replication/walsender.c   | 26 +++
 src/backend/utils/init/postinit.c | 11 
 src/backend/utils/misc/guc_tables.c   | 11 
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 doc/src/sgml/config.sgml  | 23 
 7 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/src/include/replication/walsender.h b/src/include/replication/walsender.h
index 1b58d50b3b..f4d3c73f4d 100644
--- a/src/include/replication/walsender.h
+++ b/src/include/replication/walsender.h
@@ -31,6 +31,7 @@ extern PGDLLIMPORT bool wake_wal_senders;
 /* user-settable parameters */
 extern PGDLLIMPORT int max_wal_senders;
 extern PGDLLIMPORT int wal_sender_timeout;
+extern PGDLLIMPORT int wal_sender_max_send_size;
 extern PGDLLIMPORT bool log_replication_commands;
 
 extern void InitWalSender(void);
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 5300c44f3b..b0ac07171c 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -84,6 +84,7 @@ extern bool check_maintenance_io_concurrency(int *newval, void **extra,
 extern void assign_maintenance_io_concurrency(int newval, void *extra);
 extern bool check_max_connections(int *newval, void **extra, GucSource source);
 extern bool check_max_wal_senders(int *newval, void **extra, GucSource source);
+extern bool check_wal_sender_max_send_size(int *newval, void **extra, GucSource source);
 extern bool check_max_slot_wal_keep_size(int *newval, void **extra,
 		 GucSource source);
 extern void assign_max_wal_size(int newval, void *extra);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 77c8baa32a..a55516f589 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -96,17 +96,6 @@
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
 
-/*
- * Maximum data payload in a WAL data message.  Must be >= XLOG_BLCKSZ.
- *
- * We don't have a good idea of what a good value would be; there's some
- * overhead per message in both walsender and walreceiver, but on the other
- * hand sending large batches makes walsender less responsive to signals
- * because signals are checked only between messages.  128kB (with
- * default 8k blocks) seems like a reasonable guess for now.
- */
-#define MAX_SEND_SIZE (XLOG_BLCKSZ * 16)
-
 /* Array of WalSnds in shared memory */
 WalSndCtlData *WalSndCtl = NULL;
 
@@ -124,6 +113,8 @@ int			max_wal_senders = 10;	/* the maximum number of concurrent
 	 * walsenders */
 int			wal_sender_timeout = 60 * 1000; /* maximum time to send one WAL
 			 * data message */
+int			wal_sender_max_send_size = XLOG_BLCKSZ * 16;	/* Maximum data payload
+			 * in a WAL data message */
 bool		log_replication_commands = false;
 
 /*
@@ -2950,8 +2941,8 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
 /*
  * Send out the WAL in its normal physical/stored form.
  *
- * Read up to MAX_SEND_SIZE bytes of WAL that's been flushed to disk,
- * but not yet sent to the client, and buffer it in the libpq output
+ * Read up to wal_sender_max_send_size bytes of WAL that's been flushed to
+ * disk, but not yet sent to the client, and buffer it in the libpq output
  * buffer.
  *
  * If there is no unsent WAL remaining, WalSndCaughtUp is set to true,
@@ -3132,8 +3123,9 @@ XLogSendPhysical(void)
 
 	/*
 	 * Figure out how much to send in one message. If there's no more than
-	 * MAX_SEND_SIZE bytes to send, send everything. Otherwise send
-	 * MAX_SEND_SIZE bytes, but round back to logfile or page boundary.
+	 * wal_sender_max_send_size bytes to send, send everything. Otherwise send
+	 * wal_sender_max_send_size bytes, but round back to logfile or page
+	 * boundary.
 	 *
 	 * The rounding is not only for performance reasons. Walreceiver relies on
 	 * the fact that we never split a WAL record 

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

[Michael Paquier]

On Fri, Feb 09, 2024 at 01:19:50PM +0900, Sutou Kouhei wrote:
> Are you already working on this? Do you want me to write the
> next patch based on the current master?

No need for a new patch, thanks.  I've spent some time today doing a
rebase and measuring the whole, without seeing a degradation with what
should be the worst cases for COPY TO and FROM:
https://www.postgresql.org/message-id/ZcWoTr1N0GELFA9E%40paquier.xyz
--
Michael


signature.asc
Description: PGP signature

Re: Synchronizing slots from primary to standby

[Amit Kapila]

On Fri, Feb 9, 2024 at 10:00 AM Zhijie Hou (Fujitsu)
 wrote:
>
> Here is the V82 patch set which includes the following changes:
>

+reserve_wal_for_local_slot(XLogRecPtr restart_lsn)
{
...
+ /*
+ * Find the oldest existing WAL segment file.
+ *
+ * Normally, we can determine it by using the last removed segment
+ * number. However, if no WAL segment files have been removed by a
+ * checkpoint since startup, we need to search for the oldest segment
+ * file currently existing in XLOGDIR.
+ */
+ oldest_segno = XLogGetLastRemovedSegno() + 1;
+
+ if (oldest_segno == 1)
+ {
+ TimeLineID cur_timeline;
+
+ GetWalRcvFlushRecPtr(NULL, _timeline);
+ oldest_segno = XLogGetOldestSegno(cur_timeline);
...
...

This means that if the restart_lsn of the slot is from the prior
timeline then the standby needs to wait for longer times to sync the
slot. Ideally, it should be okay because I don't think even if
restart_lsn of the slot may be from some prior timeline than the
current flush timeline on standby, how often that case can happen?
OTOH, in the prior version patch(v80_2-0001*), we search for the
oldest segment in all possible timelines via code like:
+reserve_wal_for_local_slot(XLogRecPtr restart_lsn)
{
...
+ */
+ oldest_segno = XLogGetLastRemovedSegno() + 1;
+
+ if (oldest_segno == 1)
+ oldest_segno = XLogGetOldestSegno(0);

I don't see a problem either way as in both scenarios this is a very
rare case and doesn't seem to cause any problem but would like to know
the opinion of others.

-- 
With Regards,
Amit Kapila.

Re: Synchronizing slots from primary to standby

[Peter Smith]

Here are some review comments for patch v81-0001.

==

1. GENERAL - ReplicationSlotInvalidationCause enum.

I was thinking that the ReplicationSlotInvalidationCause should
explicitly set RS_INVAL_NONE = 0 (it's zero anyway, but making it
explicit with a comment /* Must be zero. */. will stop it from being
changed in the future).

--
/*
 * Slots can be invalidated, e.g. due to max_slot_wal_keep_size. If so, the
 * 'invalidated' field is set to a value other than _NONE.
 */
typedef enum ReplicationSlotInvalidationCause
{
  RS_INVAL_NONE = 0, /* Must be zero. */
  ...
} ReplicationSlotInvalidationCause;
--

The reason to do this is because many places in the patch check for
RS_INVAL_NONE, but if RS_INVAL_NONE == 0 is assured, all those code
fragments can be simplified and IMO also become more readable.

e.g. update_local_synced_slot()

BEFORE
Assert(slot->data.invalidated == RS_INVAL_NONE);

AFTER
Assert(!slot->data.invalidated);

~

e.g. local_sync_slot_required()

BEFORE
locally_invalidated =
  (remote_slot->invalidated == RS_INVAL_NONE) &&
  (local_slot->data.invalidated != RS_INVAL_NONE);

AFTER
locally_invalidated = !remote_slot->invalidated && local_slot->data.invalidated;

~

e.g. synchronize_one_slot()

BEFORE
if (slot->data.invalidated == RS_INVAL_NONE &&
 remote_slot->invalidated != RS_INVAL_NONE)

AFTER
if (!slot->data.invalidated && remote_slot->invalidated;

BEFORE
/* Skip the sync of an invalidated slot */
if (slot->data.invalidated != RS_INVAL_NONE)

AFTER
/* Skip the sync of an invalidated slot */
if (slot->data.invalidated)

BEFORE
/* Skip creating the local slot if remote_slot is invalidated already */
if (remote_slot->invalidated != RS_INVAL_NONE)

AFTER
/* Skip creating the local slot if remote_slot is invalidated already */
if (remote_slot->invalidated)

~

e.g. synchronize_slots()

BEFORE
if ((XLogRecPtrIsInvalid(remote_slot->restart_lsn) ||
  XLogRecPtrIsInvalid(remote_slot->confirmed_lsn) ||
  !TransactionIdIsValid(remote_slot->catalog_xmin)) &&
  remote_slot->invalidated == RS_INVAL_NONE)

AFTER
if ((XLogRecPtrIsInvalid(remote_slot->restart_lsn) ||
  XLogRecPtrIsInvalid(remote_slot->confirmed_lsn) ||
  !TransactionIdIsValid(remote_slot->catalog_xmin)) &&
  !remote_slot->invalidated)

==
src/backend/replication/logical/slotsync.c

2. update_local_synced_slot

+ if (strcmp(remote_slot->plugin, NameStr(slot->data.plugin)) == 0 &&
+ remote_dbid == slot->data.database &&
+ !xmin_changed && !restart_lsn_changed &&
+ remote_slot->two_phase == slot->data.two_phase &&
+ remote_slot->failover == slot->data.failover &&
+ remote_slot->confirmed_lsn == slot->data.confirmed_flush)
+ return false;

Consider rearranging the conditions to put the strcmp later -- e.g.
might as well avoid the (more expensive?) strcmp if some of those
boolean tests are already false.

~~~

3.
+ /*
+ * There is a possibility of parallel database drop by startup
+ * process and re-creation of new slot by user in the small window
+ * between getting the slot to drop and locking the db. This new
+ * user-created slot may end up using the same shared memory as
+ * that of 'local_slot'. Thus check if local_slot is still the
+ * synced one before performing actual drop.
+ */

BEFORE
There is a possibility of parallel database drop by startup process
and re-creation of new slot by user in the small window between
getting the slot to drop and locking the db.

SUGGESTION
In the small window between getting the slot to drop and locking the
database, there is a possibility of a parallel database drop by the
startup process or the creation of a new slot by the user.

~~~

4.
+/*
+ * Synchronize single slot to given position.
+ *
+ * This creates a new slot if there is no existing one and updates the
+ * metadata of the slot as per the data received from the primary server.
+ *
+ * The slot is created as a temporary slot and stays in the same
state until the
+ * the remote_slot catches up with locally reserved position and local slot is
+ * updated. The slot is then persisted and is considered as sync-ready for
+ * periodic syncs.
+ */

/Synchronize single slot to given position./Synchronize a single slot
to the given position./

~~~

5. synchronize_slots

+ /*
+ * The primary_slot_name is not set yet or WALs not received yet.
+ * Synchronization is not possible if the walreceiver is not started.
+ */
+ latestWalEnd = GetWalRcvLatestWalEnd();
+ SpinLockAcquire(>mutex);
+ if ((WalRcv->slotname[0] == '\0') ||
+ XLogRecPtrIsInvalid(latestWalEnd))
+ {
+ SpinLockRelease(>mutex);
+ return;
+ }
+ SpinLockRelease(>mutex);

The comment talks about the GUC "primary_slot_name", but the code is
checking the WalRcv's slotname. It may be the same, but the difference
is confusing.

~~~

6.
+ /*
+ * If restart_lsn, confirmed_lsn or catalog_xmin is invalid but slot
+ * is not invalidated, that means we have fetched the remote_slot in
+ * its RS_EPHEMERAL state itself. In such a case, avoid syncing it
+ * yet. We can always sync it in 

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

[Michael Paquier]

On Wed, Feb 07, 2024 at 01:33:18PM +0900, Michael Paquier wrote:
> Hmm.  That explains why I was not seeing any differences with this
> callback then.  It seems to me that the order of actions to take is
> clear, like:
> - Revert 2889fd23be56 to keep a clean state of the tree, now done with
> 1aa8324b81fa.
> - Dive into the strlen() issue, as it really looks like this can
> create more simplifications for the patch discussed on this thread
> with COPY TO.

This has been done this morning with b619852086ed.

> - Revisit what we have here, looking at more profiles to see how HEAD
> an v13 compare.  It looks like we are on a good path, but let's tackle
> things one step at a time.

And attached is a v14 that's rebased on HEAD.  While on it, I've
looked at more profiles and did more runtime checks.

Some runtimes, in (ms), average of 15 runs, 30 int attributes on 5M
rows as mentioned above:
COPY FROM  text   binary
HEAD   6066   7110
v146087   7105
COPY TOtext   binary
HEAD   6591   10161
v146508   10189

And here are some profiles, where I'm not seeing an impact at
row-level with the addition of the callbacks:
COPY FROM, text, master:
-   66.59%16.10%  postgres  postgres[.] NextCopyFrom

   ▒- 50.50% NextCopyFrom
   - 30.75% NextCopyFromRawFields
  + 15.93% CopyReadLine
13.73% CopyReadAttributesText
   - 19.43% InputFunctionCallSafe
  + 13.49% int4in
0.77% pg_strtoint32_safe
+ 16.10% _start
COPY FROM, text, v14:
-   66.42% 0.74%  postgres  postgres[.] NextCopyFrom
- 65.67% NextCopyFrom
   - 65.51% CopyFromTextOneRow
  - 30.25% NextCopyFromRawFields
 + 16.14% CopyReadLine
   13.40% CopyReadAttributesText
  - 18.96% InputFunctionCallSafe
 + 13.15% int4in
   0.70% pg_strtoint32_safe
+ 0.74% _start

COPY TO, binary, master
-   90.32% 7.14%  postgres  postgres[.] CopyOneRowTo
- 83.18% CopyOneRowTo
   + 60.30% SendFunctionCall
   + 10.99% appendBinaryStringInfo
   + 3.67% MemoryContextReset
   + 2.89% CopySendEndOfRow
 0.89% memcpy@plt
 0.66% 0xa052db5c
 0.62% enlargeStringInfo
 0.56% pgstat_progress_update_param
+ 7.14% _start
COPY TO, binary, v14
-   90.96% 0.21%  postgres  postgres[.] CopyOneRowTo
- 90.75% CopyOneRowTo
   - 81.86% CopyToBinaryOneRow
  + 59.17% SendFunctionCall
  + 10.56% appendBinaryStringInfo
1.10% enlargeStringInfo
0.59% int4send
0.57% memcpy@plt
   + 3.68% MemoryContextReset
   + 2.83% CopySendEndOfRow
 1.13% appendBinaryStringInfo
 0.58% SendFunctionCall
 0.58% pgstat_progress_update_param

Are there any comments about this v14?  Sutou-san?

A next step I think we could take is to split the binary-only and the
text/csv-only data in each cstate into their own structure to make the
structure, with an opaque pointer that custom formats could use, but a
lot of fields are shared as well.  This patch is already complicated
enough IMO, so I'm OK to leave it out for the moment, and focus on
making this infra pluggable as a next step.
--
Michael
From 6aa7f8cb65e792def8dda631fed19404db0a88af Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 6 Feb 2024 08:40:17 +0900
Subject: [PATCH v14] Extract COPY FROM/TO format implementations

This doesn't change the current behavior.  This just introduces a set of
copy routines called CopyFromRoutine and CopyToRoutine, which just has
function pointers of format implementation like TupleTableSlotOps, and
use it for existing "text", "csv" and "binary" format implementations.
There are plans to extend that more in the future with custom formats.

This improves performance when a relation has many attributes as this
eliminates format-based if branches.  The more the attributes, the
better the performance gain.  Blah add some numbers.
---
 src/include/commands/copyapi.h   | 100 ++
 src/include/commands/copyfrom_internal.h |  10 +
 src/backend/commands/copyfrom.c  | 209 ---
 src/backend/commands/copyfromparse.c | 362 ++-
 src/backend/commands/copyto.c| 434 ---
 src/tools/pgindent/typedefs.list |   2 +
 6 files changed, 770 insertions(+), 347 deletions(-)
 create mode 100644 src/include/commands/copyapi.h

diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
new file mode 100644
index 00..87e0629c2f
--- /dev/null
+++ b/src/include/commands/copyapi.h
@@ -0,0 +1,100 @@
+/*-
+ *
+ * copyapi.h
+ *	  API for COPY TO/FROM handlers
+ *
+ *
+ * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group

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

[Sutou Kouhei]

Hi,

In 
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on 
Wed, 7 Feb 2024 13:33:18 +0900,
  Michael Paquier  wrote:

> Hmm.  That explains why I was not seeing any differences with this
> callback then.  It seems to me that the order of actions to take is
> clear, like:
> - Revert 2889fd23be56 to keep a clean state of the tree, now done with
> 1aa8324b81fa.

Done.

> - Dive into the strlen() issue, as it really looks like this can
> create more simplifications for the patch discussed on this thread
> with COPY TO.

Done: b619852086ed2b5df76631f5678f60d3bebd3745

> - Revisit what we have here, looking at more profiles to see how HEAD
> an v13 compare.  It looks like we are on a good path, but let's tackle
> things one step at a time.

Are you already working on this? Do you want me to write the
next patch based on the current master?


Thanks,
-- 
kou

Re: Psql meta-command conninfo+

[Nathan Bossart]

Sorry if this has been brought up, but I noticed that some of the
information is listed below the result set:

postgres=# \conninfo+
Current Connection Information
-[ RECORD 1 ]--+-
Database   | postgres
Authenticated User | nathan
System User| 
Current User   | nathan
Session User   | nathan
Session PID| 659410
Server Version | 17devel
Server Address | ::1
Server Port| 5432
Client Address | ::1
Client Port| 59886
Socket Directory   | 
Host   | ::1

SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384, 
compression: off)

Shouldn't we move this information into the result set?  Separately, does
the server version really belong here?  I'm not sure I would consider that
to be connection information.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability

[Jeevan Chalke]

On Thu, Feb 8, 2024 at 2:22 PM jian he  wrote:

> On Thu, Feb 8, 2024 at 1:27 PM Jeevan Chalke
>  wrote:
> >
> >
> >
> > On Wed, Feb 7, 2024 at 9:13 PM jian he 
> wrote:
> >>
> >> On Wed, Feb 7, 2024 at 7:36 PM Jeevan Chalke
> >>  wrote:
> >> > Added checkTimezoneIsUsedForCast() check where ever we are casting
> timezoned to non-timezoned types and vice-versa.
> >>
> >> https://www.postgresql.org/docs/devel/functions-json.html
> >> above Table 9.51. jsonpath Filter Expression Elements, the Note
> >> section, do we also need to rephrase it?
> >
> >
> > OK. Added a line for the same.
> >
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index 6788ba8..37ae2d1 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -18240,7 +18240,11 @@ ERROR:  jsonpath member accessor can only be
> applied to an object
>timestamptz, and time to
> timetz.
>However, all but the first of these conversions depend on the
> current
> setting, and thus can only be
> performed
> -  within timezone-aware jsonpath functions.
> +  within timezone-aware jsonpath functions.  Similarly,
> other
> +  date/time-related methods that convert string to the date/time types
> +  also do the casting and may involve the current
> +  .  To preserve the immutability,
> those can
> +  only be performed within timezone-aware jsonpath
> functions.
>   
>  
>
> my proposed minor changes:
> -  within timezone-aware jsonpath functions.
> +  within timezone-aware jsonpath functions. Similarly,
> other
> +  date/time-related methods that convert string to the date/time types
> +  also do the casting and may involve the current
> +   setting. Those conversions can
> +  only be performed within timezone-aware jsonpath
> functions.
> I don't have a strong opinion, though.
>

That seems fine as well. Let's leave that to the committer.


Thanks
-- 
Jeevan Chalke

*Principal, ManagerProduct Development*



edbpostgres.com

RE: Psql meta-command conninfo+

[Maiquel Grassi]

On 08.02.24 21:37, Erik Wienhold wrote:
>> Modifiers such as + or S in \dS are not covered by autocompletion.
>> src/bin/psql/tab-complete.c only specifies backslash commands in their
>> basic form (without modifiers).
>>
>> (\dS actually autocompletes to \ds to my surprise)
>>
>Aha... I never noticed it. Well, with most commands having 1 - 3
>characters it is not a surprised I never used it :)
>That "\dS" autocompletes to "\ds " surprises me even more.
>Thanks for pointing out!

--//--

Good evening, dear all!


Here is the mechanism that implements this:

https://github.com/postgres/postgres/blob/b619852086ed2b5df76631f5678f60d3bebd3745/src/bin/psql/tab-complete.h

.
.
.
1673   /* Match the last N words before point, case-sensitively. */

1674 #define TailMatchesCS(...) \
1675   TailMatchesImpl(true, previous_words_count, previous_words, \

1676   VA_ARGS_NARGS(__VA_ARGS__), __VA_ARGS__)
.
.
.
4824   else if (TailMatchesCS("\\ds*"))
4825 COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
.
.
.

There is a rather large list of meta-commands that are handled by 
TailMatchesCS(...).

For example:
\ENCODING autocompletes to \encoding
\eNcOdInG autocompletes to \encoding
\dU or \DU autocompletes to \du

Including the command under discussion:
\CONNINFO autocompletes to \conninfo

For the meta-commands[+], there is no autocomplete.

Regards,
Maiquel Grassi.

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

[Michael Paquier]

On Thu, Feb 01, 2024 at 10:57:58AM +0900, Michael Paquier wrote:
> CREATE EXTENSION blackhole_am;

One thing I have forgotten here is to provide a copy of this AM for
future references, so here you go with a blackhole_am.tar.gz attached.
--
Michael


blackhole_am.tar.gz
Description: application/gzip


signature.asc
Description: PGP signature

Re: Where can I find the doxyfile?

[John Morris]

>> We also have a handful of .cpp files.
Fortunately, our .cpp files are simple and do not include C++ specific features.
We shouldn’t have any problem adding .cpp to the filter list.

At some point it will be necessary to support general C++, but I wasn’t 
planning to do it yet.

>> This file isn't really following postgres coding style - any reason not to 
>> do so?
I’ll update it to Postgres commenting. The filter started as a standalone 
project.

>> What do you mean with "build time error messages" specifically? Why would we
>> want to output anything at build time (rather than configure time)?
“build time error messages” occur after typing “ninja”.

Documentation is usually produced long after meson setup messages have been 
forgotten.
It is much more useful to see “Please install the doxygen program” after typing 
“ninja doxygen”
than to see “target not supported”, or to have to look through the setup log.

It’s personal preference, but I think the sgml docs should display a similar 
message.

>> I'd add "native: true", it'd not work to find the program in the target
>> environment of a cross build.
Got it. Thanks.

Re: confusing / inefficient "need_transcoding" handling in copy

[Andres Freund]

Hi,

On 2024-02-09 09:36:28 +0900, Michael Paquier wrote:
> On Thu, Feb 08, 2024 at 10:25:07AM +0200, Heikki Linnakangas wrote:
> > There's no validation, just conversion. I'd suggest:
> > 
> > "Set up encoding conversion info if the file and server encodings differ
> > (see also pg_server_to_any)."
> > 
> > Other than that, +1
> 
> Cool.  I've used your wording and applied that on HEAD.

Thanks. LGTM.

Greetings,

Andres Freund

Re: confusing / inefficient "need_transcoding" handling in copy

[Michael Paquier]

On Thu, Feb 08, 2024 at 10:25:07AM +0200, Heikki Linnakangas wrote:
> There's no validation, just conversion. I'd suggest:
> 
> "Set up encoding conversion info if the file and server encodings differ
> (see also pg_server_to_any)."
> 
> Other than that, +1

Cool.  I've used your wording and applied that on HEAD.

> BTW, I can see an optimization opportunity even if the encodings differ:
> Currently, CopyAttributeOutText() calls pg_server_to_any(), and then grovels
> through the string to find any characters that need to be quoted. You could
> do it the other way round and handle quoting before the conversion. That has
> two benefits:
> 
> 1. You don't need the strlen() call, because you just scanned through the
> string so you already know its length.
> 2. You don't need to worry about 'encoding_embeds_ascii' when you operate on
> the server encoding.

That sounds right, still it looks like there would be cases where
you'd need the strlen() call if !encoding_embeds_ascii.
--
Michael


signature.asc
Description: PGP signature

Re: Psql meta-command conninfo+

[Jim Jones]

Hi Erik

On 08.02.24 21:37, Erik Wienhold wrote:
> Modifiers such as + or S in \dS are not covered by autocompletion.
> src/bin/psql/tab-complete.c only specifies backslash commands in their
> basic form (without modifiers).
>
> (\dS actually autocompletes to \ds to my surprise)
>
Aha... I never noticed it. Well, with most commands having 1 - 3
characters it is not a surprised I never used it :)
That "\dS" autocompletes to "\ds " surprises me even more.
Thanks for pointing out!

-- 
Jim

Re: confusing / inefficient "need_transcoding" handling in copy

[Michael Paquier]

On Thu, Feb 08, 2024 at 05:29:46PM +0900, Sutou Kouhei wrote:
> Oh, sorry. I missed the Michael's patch:
> https://www.postgresql.org/message-id/flat/ZcR9Q9hJ8GedFSCd%40paquier.xyz#e73272b042a22befac7a95f7bcb4fb9a
> 
> I withdraw my change.

No problem.  Thanks for caring about that.
--
Michael


signature.asc
Description: PGP signature

Re: Make documentation builds reproducible

[Peter Smith]

On Thu, Feb 8, 2024 at 9:47 PM Peter Eisentraut  wrote:
>
> On 23.01.24 02:06, Peter Smith wrote:
> > This has been working forever, but seems to have broken due to commit
> > [1] having an undeclared variable.
>
> > runtime error: file stylesheet-html-common.xsl line 452 element if
> > Variable 'autolink.index.see' has not been declared.
> > make: *** [html-stamp] Error 10
>
> I have committed a fix for this.  I have successfully tested docbook-xsl
> 1.77.1 through 1.79.*.
>

Yes, the latest is working for me now. Thanks!

==
Kind Regards,
Peter Smith.
Fujitsu Australia.

Re: Reducing connection overhead in pg_upgrade compat check phase

[Daniel Gustafsson]

> On 8 Feb 2024, at 15:16, Daniel Gustafsson  wrote:

> One option could perhaps be to include a version number for <= comparison, and
> if set to zero a function pointer to a version check function must be 
> provided?
> That would handle the simple cases in a single place without messy logic, and
> leave the more convoluted checks with a special case function.

The attached is a draft version of this approach, each check can define to run
for all versions, set a threshold version for which it runs or define a
callback which implements a more complicated check.

--
Daniel Gustafsson



v13-0001-pg_upgrade-run-all-data-type-checks-per-connecti.patch
Description: Binary data

RE: Psql meta-command conninfo+

[Maiquel Grassi]

>On 2024-02-08 20:37 +0100, Jim Jones wrote:
>> One thing I just noticed. The psql autocomplete feature does not suggest
>> the new + option of \conninfo. For instance, when typing "\connin[TAB]"
>> it automatically autocompletes to "\conninfo ". I guess it should also
>> be included in this patch.
>
>Modifiers such as + or S in \dS are not covered by autocompletion.
>src/bin/psql/tab-complete.c only specifies backslash commands in their
>basic form (without modifiers).
>
>(\dS actually autocompletes to \ds to my surprise)
>
>> I can do a more thorough review of the code when you add the
>> documentation and tests to the patch.
>
>I noticed that the pattern parameter in listConnectionInformation is
>unused.  exec_command_conninfo scans the pattern but \conninfo should
>not accept any argument.  So the pattern can be removed entirely.

--//--

Hi Erik,


Exactly, in "\conninfo+" the "pattern" argument was not introduced and
therefore "listConnectionInformation()" can be declared (signed) without
any arguments. In the future, if necessary, and if there is a need to add
any "pattern", then that can be done. However, that's not the current
scenario. I removed everything related to the "pattern" from the patch.

Regarding "(\dS actually autocompletes to \ds to my surprise)",
I was also surprised. I haven't studied the code yet to understand why
this happens. It seems curious to me. I'll try to understand this
implementation better.

I'm continuing the development of "\conninfo+" and now moving on to tests.

Tks a lot!

Regards,
Maiquel Grassi.


v10-0001-psql-meta-command-conninfo-plus.patch
Description: v10-0001-psql-meta-command-conninfo-plus.patch

Re: "ERROR: latch already owned" on gharial

[Andres Freund]

Hi,

On 2024-02-08 14:57:47 +0200, Heikki Linnakangas wrote:
> On 08/02/2024 04:08, Soumyadeep Chakraborty wrote:
> > A possible ordering of events:
> > 
> > (1) DisownLatch() is called by pid Y during ProcKill() and the write for
> > latch->owner_pid = 0 is NOT yet flushed to shmem.
> > 
> > (2) The PGPROC object for pid Y is returned to the free list.
> > 
> > (3) Pid X sees the same PGPROC object on the free list and grabs it.
> > 
> > (4) Pid X does sanity check inside OwnLatch during InitProcess and
> > still sees the
> > old value of latch->owner_pid = Y (and not = 0), and trips the ERROR.
> > 
> > The above sequence of operations should apply to PG HEAD as well.
> > 
> > Suggestion:
> > 
> > Should we do a pg_memory_barrier() at the end of DisownLatch(), like in
> > ResetLatch(), like the one introduced in [3]? This would ensure that the 
> > write
> > latch->owner_pid = 0; is flushed to shmem. The attached patch does this.
> 
> Hmm, there is a pair of SpinLockAcquire() and SpinLockRelease() in
> ProcKill(), before step 3 can happen.

Right.  I wonder if the issue istead could be something similar to what was
fixed in 8fb13dd6ab5b and more generally in 97550c0711972a. If two procs go
through proc_exit() for the same process, you can get all kinds of weird
mixed up resource ownership.  The bug fixed in 8fb13dd6ab5b wouldn't apply,
but it's pretty easy to introduce similar bugs in other places, so it seems
quite plausible that greenplum might have done so.  We also did have more
proc_exit()s in signal handlers in older branches, so it might just be an
issue that also was present before.

Greetings,

Andres Freund

Re: Where can I find the doxyfile?

[Andres Freund]

Hi,

On 2024-02-08 18:30:01 +, John Morris wrote:
> +FILTER_PATTERNS= *.c *.h

We also have a handful of .cpp files.

> +/*
> + * Custom banner character to be removed from comments.
> + * We'll hardcode it to suit postgreSQL, but it should be set through a 
> command line arg.
> + */
> +char customBanner = '-';
> +
> +/*
> +A simple program which reads a file, updates the comments, and writes to 
> stdout.
> +This is intended to be used as a doxygen filter, converting existing 
> comments to doxygen comments.
> +**/
> +int main(int argc, char**argv) {
> +
> +// Verify we have a single argument.

This file isn't really following postgres coding style - any reason not to do
so?



> diff --git a/doc/doxygen/meson.build b/doc/doxygen/meson.build
> new file mode 100644
> index 00..e5539b7854
> --- /dev/null
> +++ b/doc/doxygen/meson.build
> @@ -0,0 +1,73 @@
> +# Generate doxygen pages for PostgreSQL using "ninja doxygen"
> +#
> +# Doxygen pages are optional. Nothing in this script should
> +# cause PostgreSQL builds to fail.
> +#
> +# Currently there are no explicit error messages
> +#   - If doxygen is not found, the doxygen target will not be defined.
> +#   - If dot is not found, no graphs will be generated.
> +#   - flex is already required, so we don't check for it.
> +#
> +# As a future enhancement, display meaningful error messages
> +# when doxygen or dot are not found. Meson does not
> +# support build time error messages, but they can be displayed
> +# using a python custom target.

What do you mean with "build time error messages" specifically? Why would we
want to output anything at build time (rather than configure time)?


> +# Find the doxygen command. If not found, stop and don't define the target.
> +doxygen_cmd = find_program('doxygen', required: false)

I'd add "native: true", it'd not work to find the program in the target
environment of a cross build.

> +
> +# Find the dot command. If not found, no graphs will be generated.
> +dot_cmd = find_program('dot', required: false)

Dito.


Greetings,

Andres Freund

RE: Psql meta-command conninfo+

[Maiquel Grassi]

> v8 no longer throws a permission denied error for non-superusers - it is
> IMHO much nicer this way.
>
> $ /usr/local/postgres-dev/bin/psql postgres -p 5432 -h 127.0.0.1 -U jim
> psql (17devel)
> Type "help" for help.
>
> postgres=# \x
> Expanded display is on.
> postgres=# \conninfo+
> Current Connection Information
> -[ RECORD 1 ]--+--
> Database   | postgres
> Authenticated User | jim
> System User|
> Current User   | jim
> Session User   | jim
> Session PID| 1321493
> Server Version | 17devel
> Server Address | 127.0.0.1
> Server Port| 5432
> Client Address | 127.0.0.1
> Client Port| 49366
> Socket Directory   |
> Host   | 127.0.0.1
>
> postgres=# SET ROLE foo;
> SET
> postgres=> \conninfo+
> Current Connection Information
> -[ RECORD 1 ]--+--
> Database   | postgres
> Authenticated User | jim
> System User|
> Current User   | foo
> Session User   | jim
> Session PID| 1321493
> Server Version | 17devel
> Server Address | 127.0.0.1
> Server Port| 5432
> Client Address | 127.0.0.1
> Client Port| 49366
> Socket Directory   |
> Host   | 127.0.0.1
>
> The patch now applies cleanly.
>
> One thing I just noticed. The psql autocomplete feature does not suggest
> the new + option of \conninfo. For instance, when typing "\connin[TAB]"
> it automatically autocompletes to "\conninfo ". I guess it should also
> be included in this patch.
>
> I can do a more thorough review of the code when you add the
> documentation and tests to the patch.
>
> Thanks!

--//--

Hi Jim,


It's not a psql standard to use tab-complete for commands ending with +.
You can verify this in the /* psql's backslash commands. */ section of
the file "/src/bin/psql/tab-complete.c".

https://github.com/postgres/postgres/blob/fdfb92c0307c95eba10854196628d88e6708901e/src/bin/psql/tab-complete.c

In v9, I started the documentation work and am open to suggestions.


> "I can do a more thorough review of the code when you add the

> documentation and tests to the patch."

Soon I'll be developing the tests. And that will be welcome.

Thanks a lot!

Regards,
Maiquel Grassi.


v9-0001-psql-meta-command-conninfo-plus.patch
Description: v9-0001-psql-meta-command-conninfo-plus.patch

Re: glibc qsort() vulnerability

[Tom Lane]

Nathan Bossart  writes:
> On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:
>> I'd put these static inlines into common/int.h. I don't think this is common
>> enough to warrant being in c.h. Probably also doesn't hurt to have a not 
>> quite
>> as generic name as INT_CMP, I'd not be too surprised if that's defined in 
>> some
>> library.
>> 
>> I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
>> in the name, an efficient implementation for signed might not be the same as
>> for unsigned. And if we use static inlines, we need to do so for correct
>> semantics anyway.

> Seems reasonable to me.

+1 here also.

regards, tom lane

Re: Psql meta-command conninfo+

[Erik Wienhold]

On 2024-02-08 20:37 +0100, Jim Jones wrote:
> One thing I just noticed. The psql autocomplete feature does not suggest
> the new + option of \conninfo. For instance, when typing "\connin[TAB]"
> it automatically autocompletes to "\conninfo ". I guess it should also
> be included in this patch.

Modifiers such as + or S in \dS are not covered by autocompletion.
src/bin/psql/tab-complete.c only specifies backslash commands in their
basic form (without modifiers).

(\dS actually autocompletes to \ds to my surprise)

> I can do a more thorough review of the code when you add the
> documentation and tests to the patch.

I noticed that the pattern parameter in listConnectionInformation is
unused.  exec_command_conninfo scans the pattern but \conninfo should
not accept any argument.  So the pattern can be removed entirely.

-- 
Erik

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 9:07 PM Nathan Bossart 
wrote:

> On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:
> > On 2024-02-08 13:44:02 -0500, Tom Lane wrote:
> >> Are we okay with using macros that (a) have double evaluation hazards
> >> and (b) don't enforce the data types being compared are the same?
> >> I think static inlines might be a safer technology.
> >
> > +1
>
> Agreed on static inlines.
>

Seems to be a general consensus on static inlines. I'll get a new patch.


> > I'd put these static inlines into common/int.h. I don't think this is
> common
> > enough to warrant being in c.h. Probably also doesn't hurt to have a not
> quite
> > as generic name as INT_CMP, I'd not be too surprised if that's defined
> in some
> > library.
> >
> >
> > I think it's worth following int.h's pattern of including
> [s]igned/[u]nsigned
> > in the name, an efficient implementation for signed might not be the
> same as
> > for unsigned. And if we use static inlines, we need to do so for correct
> > semantics anyway.
>
> Seems reasonable to me.
>

Agree.

Best wishes,
Mats Kindahl


>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 7:44 PM Tom Lane  wrote:

> Nathan Bossart  writes:
> > On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:
> >> +/*
> >> + * Compare two integers and return -1, 0, or 1 without risking
> overflow.
> >> + *
> >> + * This macro is used to avoid running into overflow issues because a
> simple
> >> + * subtraction of the two values when implementing a cmp function for
> qsort().
> >> +*/
> >> +#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))
>
> > I think we should offer a few different macros, i.e., separate macros for
> > int8, uint8, int16, uint16, int32, etc.  For int16, we can do something
> > faster like
>
> >   (int32) (lhs) - (int32) (rhs)
>
> > but for int32, we need to do someting more like what's in the patch.
>
> Are we okay with using macros that (a) have double evaluation hazards
> and (b) don't enforce the data types being compared are the same?
> I think static inlines might be a safer technology.  Perhaps it's
> okay given the only expected use is in qsort comparators, but ...
>

I picked a macro simply because it can deal with all kinds of integers, but
if we want to have separate implementations for each then inline functions
work just as well and will be safer.

 Best wishes,
Mats Kindahl


> regards, tom lane
>

Re: pg_get_expr locking

[Tom Lane]

I wrote:
> Peter Eisentraut  writes:
>> I think the situation is that one test (domain) runs pg_get_expr as part 
>> of an information_schema view, while at the same time another test 
>> (alter_table) drops a table that the pg_get_expr is just processing.

> The test case that's failing is, IIUC,

> +SELECT * FROM information_schema.domain_constraints
> +  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
> +  ORDER BY constraint_name;

Oh, scratch that: there are two confusingly lookalike queries
in the patch.  The one that is failing is

SELECT * FROM information_schema.check_constraints
  WHERE (constraint_schema, constraint_name)
IN (SELECT constraint_schema, constraint_name
FROM information_schema.domain_constraints
WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
  ORDER BY constraint_name;

and we have trouble because the evaluation of pg_get_expr in
check_constraints is done before the semijoin that would restrict
it to just the desired objects.

After looking at the code I'm less worried about the
permissions-checking angle than I was before, because I see
that pg_get_expr already takes a transient AccessShareLock
on the rel, down inside set_relation_column_names.  This is
not ideal from a permissions standpoint perhaps, but it's
probably OK considering we've done that for a long time.
We just need to hold that lock a little while longer.

I propose the attached as a reasonably localized fix.
We could imagine a more aggressive refactoring that would
allow passing down the Relation instead of re-opening it
in set_relation_column_names, but I doubt it's worth the
trouble.

regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b625f471a8..644966721f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -352,8 +352,7 @@ static char *pg_get_partkeydef_worker(Oid relid, int prettyFlags,
 	  bool attrsOnly, bool missing_ok);
 static char *pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 		 int prettyFlags, bool missing_ok);
-static text *pg_get_expr_worker(text *expr, Oid relid, const char *relname,
-int prettyFlags);
+static text *pg_get_expr_worker(text *expr, Oid relid, int prettyFlags);
 static int	print_function_arguments(StringInfo buf, HeapTuple proctup,
 	 bool print_table_args, bool print_defaults);
 static void print_function_rettype(StringInfo buf, HeapTuple proctup);
@@ -2615,6 +2622,11 @@ decompile_column_index_array(Datum column_index_array, Oid relId,
  * partial indexes, column default expressions, etc.  We also support
  * Var-free expressions, for which the OID can be InvalidOid.
  *
+ * If the OID is nonzero but not actually valid, don't throw an error,
+ * just return NULL.  This is a bit questionable, but it's what we've
+ * done historically, and it can help avoid unwanted failures when
+ * examining catalog entries for just-deleted relations.
+ *
  * We expect this function to work, or throw a reasonably clean error,
  * for any node tree that can appear in a catalog pg_node_tree column.
  * Query trees, such as those appearing in pg_rewrite.ev_action, are
@@ -2627,29 +2639,16 @@ pg_get_expr(PG_FUNCTION_ARGS)
 {
 	text	   *expr = PG_GETARG_TEXT_PP(0);
 	Oid			relid = PG_GETARG_OID(1);
+	text	   *result;
 	int			prettyFlags;
-	char	   *relname;
 
 	prettyFlags = PRETTYFLAG_INDENT;
 
-	if (OidIsValid(relid))
-	{
-		/* Get the name for the relation */
-		relname = get_rel_name(relid);
-
-		/*
-		 * If the OID isn't actually valid, don't throw an error, just return
-		 * NULL.  This is a bit questionable, but it's what we've done
-		 * historically, and it can help avoid unwanted failures when
-		 * examining catalog entries for just-deleted relations.
-		 */
-		if (relname == NULL)
-			PG_RETURN_NULL();
-	}
+	result = pg_get_expr_worker(expr, relid, prettyFlags);
+	if (result)
+		PG_RETURN_TEXT_P(result);
 	else
-		relname = NULL;
-
-	PG_RETURN_TEXT_P(pg_get_expr_worker(expr, relid, relname, prettyFlags));
+		PG_RETURN_NULL();
 }
 
 Datum
@@ -2658,33 +2657,27 @@ pg_get_expr_ext(PG_FUNCTION_ARGS)
 	text	   *expr = PG_GETARG_TEXT_PP(0);
 	Oid			relid = PG_GETARG_OID(1);
 	bool		pretty = PG_GETARG_BOOL(2);
+	text	   *result;
 	int			prettyFlags;
-	char	   *relname;
 
 	prettyFlags = GET_PRETTY_FLAGS(pretty);
 
-	if (OidIsValid(relid))
-	{
-		/* Get the name for the relation */
-		relname = get_rel_name(relid);
-		/* See notes above */
-		if (relname == NULL)
-			PG_RETURN_NULL();
-	}
+	result = pg_get_expr_worker(expr, relid, prettyFlags);
+	if (result)
+		PG_RETURN_TEXT_P(result);
 	else
-		relname = NULL;
-
-	PG_RETURN_TEXT_P(pg_get_expr_worker(expr, relid, relname, prettyFlags));
+		PG_RETURN_NULL();
 }
 
 static text *
-pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
+pg_get_expr_worker(text *expr, Oid relid, int prettyFlags)
 {
 	Node	   

Re: glibc qsort() vulnerability

[Nathan Bossart]

On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:
> On 2024-02-08 13:44:02 -0500, Tom Lane wrote:
>> Are we okay with using macros that (a) have double evaluation hazards
>> and (b) don't enforce the data types being compared are the same?
>> I think static inlines might be a safer technology.
> 
> +1

Agreed on static inlines.

> I'd put these static inlines into common/int.h. I don't think this is common
> enough to warrant being in c.h. Probably also doesn't hurt to have a not quite
> as generic name as INT_CMP, I'd not be too surprised if that's defined in some
> library.
> 
> 
> I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
> in the name, an efficient implementation for signed might not be the same as
> for unsigned. And if we use static inlines, we need to do so for correct
> semantics anyway.

Seems reasonable to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: gcc build warnings at -O3

[Alexander Korotkov]

On Wed, Feb 7, 2024 at 10:31 PM Andres Freund  wrote:
> On 2024-01-11 21:55:19 -0500, Tom Lane wrote:
> > Bharath Rupireddy  writes:
> > > Hi, I'm seeing a compiler warning with CFLAGS -O3 but not with -O2.
> >
> > > In file included from dbcommands.c:20:
> > > dbcommands.c: In function ‘createdb’:
> > > ../../../src/include/postgres.h:104:16: warning: ‘src_hasloginevt’ may
> > > be used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > Hmm, I also see that at -O3 (not at -O2) when using Fedora 39's
> > gcc 13.2.1, but *not* when using RHEL8's gcc 8.5.0.
>
> It's visible here with gcc >= 10. That's enough versions that I think we
> should care.  Interestingly enough, it seems to have recently have gotten
> fixed in gcc master (14 to be).

I managed to reproduce this warning locally.  Fixed.

--
Regards,
Alexander Korotkov

Re: glibc qsort() vulnerability

[Andres Freund]

Hi,

On 2024-02-08 13:44:02 -0500, Tom Lane wrote:
> Nathan Bossart  writes:
> > On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:
> >> +/*
> >> + * Compare two integers and return -1, 0, or 1 without risking overflow.
> >> + *
> >> + * This macro is used to avoid running into overflow issues because a 
> >> simple
> >> + * subtraction of the two values when implementing a cmp function for 
> >> qsort().
> >> +*/
> >> +#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))
>
> > I think we should offer a few different macros, i.e., separate macros for
> > int8, uint8, int16, uint16, int32, etc.  For int16, we can do something
> > faster like

+1


> > (int32) (lhs) - (int32) (rhs)
>
> > but for int32, we need to do someting more like what's in the patch.
>
> Are we okay with using macros that (a) have double evaluation hazards
> and (b) don't enforce the data types being compared are the same?
> I think static inlines might be a safer technology.

+1


I'd put these static inlines into common/int.h. I don't think this is common
enough to warrant being in c.h. Probably also doesn't hurt to have a not quite
as generic name as INT_CMP, I'd not be too surprised if that's defined in some
library.


I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
in the name, an efficient implementation for signed might not be the same as
for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.


Greetings,

Andres

Re: Psql meta-command conninfo+

[Jim Jones]

On 08.02.24 16:50, Maiquel Grassi wrote:
> Hi Jim,
> Thank you for your support on this patch!
> As I believe in its usability, I have been dedicating efforts to make
> it really interesting.
> I hadn't thought about the permissioning issue for
> "unix_socket_directories". I appreciate that.
> I thought about solving this situation using the same approach as
> \conninfo. I added the validation if (is_unixsock_path(host) &&
> !(hostaddr && *hostaddr)) in the SQL part along with an "append". In
> case of a negative result, another "append" adds NULL.
> Regarding the whitespace issue, before generating v8 patch file, I
> used pgindent to adjust each modified file. I believe it should be ok
> now. If you could verify, I'd be grateful.
> Below are the tests after adjusting for the permissioning issues:
>
> [postgres@localhost bin]$ ./psql
> psql (17devel)
> Type "help" for help.
>
> postgres=# \conninfo
> You are connected to database "postgres" as user "postgres" via socket
> in "/tmp" at port "5432".
> postgres=# \conninfo+
>                                                                      
>             Current Connection Information
>  Database | Authenticated User | System User | Current User | Session
> User | Session PID | Server Version | Server Address | Server Port |
> Client Address | Client Port | Socket Directory | Host
> --++-+--+--+-+++-++-+--+--
>  postgres | postgres           |             | postgres     | postgres
>     |       31479 | 17devel        |                | 5432        |  
>              |             | /tmp             |
> (1 row)
>
> postgres=# CREATE USER maiquel;
> CREATE ROLE
> postgres=# \q
> [postgres@localhost bin]$ ./psql -U maiquel -d postgres
> psql (17devel)
> Type "help" for help.
>
> postgres=> \conninfo
> You are connected to database "postgres" as user "maiquel" via socket
> in "/tmp" at port "5432".
> postgres=> \conninfo+
>                                                                      
>             Current Connection Information
>  Database | Authenticated User | System User | Current User | Session
> User | Session PID | Server Version | Server Address | Server Port |
> Client Address | Client Port | Socket Directory | Host
> --++-+--+--+-+++-++-+--+--
>  postgres | maiquel            |             | maiquel      | maiquel
>      |       31482 | 17devel        |                | 5432        |  
>              |             | /tmp             |
> (1 row)
>
> postgres=> \q
> [postgres@localhost bin]$ ./psql -h localhost -U maiquel -d postgres
> psql (17devel)
> Type "help" for help.
>
> postgres=> \conninfo
> You are connected to database "postgres" as user "maiquel" on host
> "localhost" (address "::1") at port "5432".
> postgres=> \conninfo+
>                                                                      
>               Current Connection Information
>  Database | Authenticated User | System User | Current User | Session
> User | Session PID | Server Version | Server Address | Server Port |
> Client Address | Client Port | Socket Directory |   Host
> --++-+--+--+-+++-++-+--+---
>  postgres | maiquel            |             | maiquel      | maiquel
>      |       31485 | 17devel        | ::1            | 5432        |
> ::1            |       47482 |                  | localhost
> (1 row)
>
> postgres=> \q
> [postgres@localhost bin]$ ./psql -h localhost
> psql (17devel)
> Type "help" for help.
>
> postgres=# \conninfo
> You are connected to database "postgres" as user "postgres" on host
> "localhost" (address "::1") at port "5432".
> postgres=# \conninfo+
>                                                                      
>               Current Connection Information
>  Database | Authenticated User | System User | Current User | Session
> User | Session PID | Server Version | Server Address | Server Port |
> Client Address | Client Port | Socket Directory |   Host
> --++-+--+--+-+++-++-+--+---
>  postgres | postgres           |             | postgres     | postgres
>     |       31488 | 17devel        | ::1            | 5432        |
> ::1            |       47484 |                  | localhost
> (1 row)
>
> Regards,
> Maiquel.

v8 no longer throws a permission denied error for non-superusers - it is
IMHO much nicer this way.

$ /usr/local/postgres-dev/bin/psql 

Re: glibc qsort() vulnerability

[Tom Lane]

Nathan Bossart  writes:
> On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:
>> +/*
>> + * Compare two integers and return -1, 0, or 1 without risking overflow.
>> + *
>> + * This macro is used to avoid running into overflow issues because a simple
>> + * subtraction of the two values when implementing a cmp function for 
>> qsort().
>> +*/
>> +#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

> I think we should offer a few different macros, i.e., separate macros for
> int8, uint8, int16, uint16, int32, etc.  For int16, we can do something
> faster like

>   (int32) (lhs) - (int32) (rhs)

> but for int32, we need to do someting more like what's in the patch.

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology.  Perhaps it's
okay given the only expected use is in qsort comparators, but ...

regards, tom lane

Re: glibc qsort() vulnerability

[Nathan Bossart]

On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:
> +/*
> + * Compare two integers and return -1, 0, or 1 without risking overflow.
> + *
> + * This macro is used to avoid running into overflow issues because a simple
> + * subtraction of the two values when implementing a cmp function for 
> qsort().
> +*/
> +#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

I think we should offer a few different macros, i.e., separate macros for
int8, uint8, int16, uint16, int32, etc.  For int16, we can do something
faster like

(int32) (lhs) - (int32) (rhs)

but for int32, we need to do someting more like what's in the patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Where can I find the doxyfile?

[John Morris]

>> I think all the explanatory messages in doc/doxygen/meson.build
>> are a bit much.  I think it's enough to just not define the target

Here is a patch with an updated meson.build as you suggested. I agree the 
messages were a bit much.
On the other hand, I would like to see clear error messages when dot or doxygen 
are not installed,
but I’ll leave that for a future discussion.


  *   John
-


doxygen-v4.patch
Description: doxygen-v4.patch


meson.build
Description: meson.build

Re: [PATCH] Add sortsupport for range types and btree_gist

[Bernd Helmle]

Am Mittwoch, dem 10.01.2024 um 22:18 +0800 schrieb jian he:
> 
> I split the original author's patch into 2.
> 1. Add GiST sortsupport function for all the btree-gist module data
> types except anyrange data type (which actually does not in this
> module)
> 2. Add GiST sortsupport function for anyrange data type.
> 

Please find attached a new version of this patch set with the following
changes/adjustments:

- Rebased to current master
- Heavily reworked *_cmp() functions to properly
decode GPT_VARKEY and GBT_KEY input.

For some datatypes the btree comparison functions were reused and the
input arguments not properly handled. This patch adds dedicated
btree_gist sortsupport comparison methods for all datatypes.

There was another patch from Andrey Borodin (thanks again for the hint)
and a deeper review done by Heikki in [1]. I've incorporated Heikkis
findings in this patch, too.

[...]

I've also updated the btree_gist documentation to reflect the default
sorted built strategy this patch introduces now.


Additionally i did some benchmarks again on this new version on the
patch. Still, index build speed improvement is quite impressive on the
dataset originally provided by Christoph Heiss (since its not available
anymore i've uploaded it here [2] again):

HEAD
(Index was built with default buffering setting)
-
REINDEX (s)  4809
CREATE INDEX (s) 4920

btree_gist sortsupport
--
REINDEX (s)573
CREATE INDEX (s)   578

I created another pgbench based custom script to measure the single
core speed of the lookup query of the bench-gist.py script. This looks
like this:

init.sql

BEGIN;

DROP TABLE IF EXISTS test_dataset;
CREATE TABLE test_dataset(keyid integer not null, id text not null,
block_range int4range);
CREATE TEMP SEQUENCE testset_seq;
INSERT INTO test_dataset SELECT nextval('testset_seq'), id, block_range
FROM test ORDER BY random() LIMIT 1;
CREATE UNIQUE INDEX ON test_dataset(keyid);

COMMIT;

bench.pgbench
-

\set keyid random(1, 1)
SELECT id, block_range FROM test_dataset WHERE keyid = :keyid \gset
SELECT id, block_range FROM test WHERE id = ':id' AND block_range &&
':block_range';

Run by

for in in `seq 1 3`; do psql -qXf init.pgbench && pgbench -n -r -c 1 -T
60 -f bench.pgbench; done

With this i get the following (on prewarmed index and table):

HEAD 
-
pgbench single core tps=248,67

btree_gist sortsupport

pgbench single core tps=1830,33

This is an average over 3 runs each (complete results attached). So
this looks really impressive and i hope i didn't do something entirely
wrong (still learning about this GiST stuff).

> what I am confused:
> In fmgr.h
> 
> /*
>  * Support for cleaning up detoasted copies of inputs.  This must
> only
>  * be used for pass-by-ref datatypes, and normally would only be used
>  * for toastable types.  If the given pointer is different from the
>  * original argument, assume it's a palloc'd detoasted copy, and
> pfree it.
>  * NOTE: most functions on toastable types do not have to worry about
> this,
>  * but we currently require that support functions for indexes not
> leak
>  * memory.
>  */
> #define PG_FREE_IF_COPY(ptr,n) \
> do { \
> if ((Pointer) (ptr) != PG_GETARG_POINTER(n)) \
> pfree(ptr); \
> } while (0)
> 
> but the doc
> (https://www.postgresql.org/docs/current/gist-extensibility.html)
>  says:
> All the GiST support methods are normally called in short-lived
> memory
> contexts; that is, CurrentMemoryContext will get reset after each
> tuple is processed. It is therefore not very important to worry about
> pfree'ing everything you palloc.
> ENDOF_QUOTE
> 
> so I am not sure in range_gist_cmp, we need the following:
> `
> if ((Datum) range_a != a)
> pfree(range_a);
> if ((Datum) range_b != b)
> pfree(range_b);
> `

Turns out this is not true for sortsupport: the comparison function is
called for each tuple during sorting, which will leak the detoasted
(and probably copied datum) in the sort memory context. See the same
for e.g. numeric and text, which i needed to change to pass the key
values correctly to the text_cmp() or numeric_cmp() function in these 
cases.

I've adapted the PG_FREE_IF_COPY() macro for these functions and
introduced GBT_FREE_IF_COPY() in btree_utils_var.h, since the former
relies on fcinfo.

I'll add the patch again to the upcoming CF for another review round.

[1]
https://www.postgresql.org/message-id/c0846e34-8b3a-e1bf-c88e-021eb241a481%40iki.fi

[2] 
https://drive.google.com/file/d/1CPNFGR53-FUto1zjXPMM2Yrn0GaGfGFz/view?usp=drive_link

HEAD
--
pgbench (17devel)
transaction type: bench.pgbench
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 60 s
number of transactions actually processed: 15192
number of failed transactions: 0 (0.000%)
latency average = 3.965 ms
initial connection time = 2.786 ms
tps 

Re: Avoiding concurrent calls to bindtextdomain()

[Tom Lane]

I wrote:
> 0001 also gets rid of the possibility that pthread_mutex_init/
> pthread_mutex_lock could fail due to malloc failure.  This seems
> important since default_threadlock() assumes that pthread_mutex_lock
> cannot fail in practice.  I observe that ecpglib also assumes that,
> although it's using CreateMutex() which has documented failure
> conditions.  So I wonder if we ought to copy this implementation
> back into ecpglib; but I've not done that here.

The cfbot seemed happy with v1, so here's a v2 that does copy that
code back into ecpglib.  (I kind of wonder why this code exists in
libpq + ecpglib at all, rather than in src/port/; but that seems
like a refactoring exercise for another day.)

I double-checked that all the pthread_mutex_t variables in libpq
and ecpglib are static, so the change in that struct should not
pose an ABI hazard for back-patching.

Barring objections, I plan to push this pretty soon.

regards, tom lane

From 8d7ebd1fc5b556850fcbe117f44f1e0918edf4b9 Mon Sep 17 00:00:00 2001
From: Tom Lane 
Date: Thu, 8 Feb 2024 12:54:08 -0500
Subject: [PATCH v2 1/2] Clean up unnecessarily Windows-dependent code in
 libpq.

Fix pthread-win32.h and pthread-win32.c to provide a more complete
emulation of POSIX pthread mutexes: define PTHREAD_MUTEX_INITIALIZER
and make sure that pthread_mutex_lock() can operate on a mutex
object that's been initialized that way.  Then we don't need the
duplicative platform-specific logic in default_threadlock() and
pgtls_init(), which we'd otherwise need yet a third copy of for
an upcoming bug fix.

Also, since default_threadlock() supposes that pthread_mutex_lock()
cannot fail, try to ensure that that's actually true, by getting
rid of the malloc call that was formerly involved in initializing
an emulated mutex.  We can define an extra state for the spinlock
field instead.

Also, replace the similar code in ecpglib/misc.c with this version.
While ecpglib's version had at least a POSIX-compliant API, it
likewise had the potential of failing during mutex init (but here,
because of CreateMutex failure rather than malloc failure).  Since
all of misc.c's callers ignore failures, it seems like a wise idea
to avoid failures here too.

A further improvement in this area could be to unify libpq's and
ecpglib's implementations into a src/port/pthread-win32.c file.
But that doesn't seem like a bug fix, so I'll desist for now.

Discussion: https://postgr.es/m/264860.1707163...@sss.pgh.pa.us
---
 src/interfaces/ecpg/ecpglib/misc.c| 37 +++
 .../ecpg/include/ecpg-pthread-win32.h | 22 ---
 src/interfaces/libpq/fe-connect.c | 16 
 src/interfaces/libpq/fe-secure-openssl.c  | 20 --
 src/interfaces/libpq/pthread-win32.c  | 26 -
 src/port/pthread-win32.h  | 11 +-
 6 files changed, 63 insertions(+), 69 deletions(-)

diff --git a/src/interfaces/ecpg/ecpglib/misc.c b/src/interfaces/ecpg/ecpglib/misc.c
index 2b78caeaf5..58fff10697 100644
--- a/src/interfaces/ecpg/ecpglib/misc.c
+++ b/src/interfaces/ecpg/ecpglib/misc.c
@@ -407,17 +407,38 @@ ECPGis_noind_null(enum ECPGttype type, const void *ptr)
 
 #ifdef WIN32
 
-void
-win32_pthread_mutex(volatile pthread_mutex_t *mutex)
+int
+pthread_mutex_init(pthread_mutex_t *mp, void *attr)
+{
+	mp->initstate = 0;
+	return 0;
+}
+
+int
+pthread_mutex_lock(pthread_mutex_t *mp)
 {
-	if (mutex->handle == NULL)
+	/* Initialize the csection if not already done */
+	if (mp->initstate != 1)
 	{
-		while (InterlockedExchange((LONG *) >initlock, 1) == 1)
-			Sleep(0);
-		if (mutex->handle == NULL)
-			mutex->handle = CreateMutex(NULL, FALSE, NULL);
-		InterlockedExchange((LONG *) >initlock, 0);
+		LONG		istate;
+
+		while ((istate = InterlockedExchange(>initstate, 2)) == 2)
+			Sleep(0);			/* wait, another thread is doing this */
+		if (istate != 1)
+			InitializeCriticalSection(>csection);
+		InterlockedExchange(>initstate, 1);
 	}
+	EnterCriticalSection(>csection);
+	return 0;
+}
+
+int
+pthread_mutex_unlock(pthread_mutex_t *mp)
+{
+	if (mp->initstate != 1)
+		return EINVAL;
+	LeaveCriticalSection(>csection);
+	return 0;
 }
 
 static pthread_mutex_t win32_pthread_once_lock = PTHREAD_MUTEX_INITIALIZER;
diff --git a/src/interfaces/ecpg/include/ecpg-pthread-win32.h b/src/interfaces/ecpg/include/ecpg-pthread-win32.h
index 8252a17809..7b6ba46b34 100644
--- a/src/interfaces/ecpg/include/ecpg-pthread-win32.h
+++ b/src/interfaces/ecpg/include/ecpg-pthread-win32.h
@@ -12,28 +12,22 @@
 
 typedef struct pthread_mutex_t
 {
-	HANDLE		handle;
-	LONG		initlock;
+	/* initstate = 0: not initialized; 1: init done; 2: init in progress */
+	LONG		initstate;
+	CRITICAL_SECTION csection;
 } pthread_mutex_t;
 
 typedef DWORD pthread_key_t;
 typedef bool pthread_once_t;
 
-#define PTHREAD_MUTEX_INITIALIZER	{ NULL, 0 }
+#define PTHREAD_MUTEX_INITIALIZER	{ 0 }
 #define PTHREAD_ONCE_INIT			false
 
-void		win32_pthread_mutex(volatile 

Re: Race condition in FetchTableStates() breaks synchronization of subscription tables

[Alexander Lakhin]

08.02.2024 12:25, vignesh C wrote:

Yes, the wakeup call sent by the "CREATE SUBSCRIPTION" command was
getting missed in this case. The wakeup call can be sent during
subscription creation/modification and when the apply worker exits.
WaitForReplicationWorkerAttach should not reset the latch here as it
will end up delaying the apply worker to get started after 180 seconds
timeout(DEFAULT_NAPTIME_PER_CYCLE). The attached patch does not reset
the latch and lets ApplyLauncherMain to reset the latch and checks if
any new worker or missing worker needs to be started.


Thank you for the updated patch!
I ran all the subscription tests in a loop (with the sleeps added as
before) and observed no failures and 180+ seconds duration.

Best regards,
Alexander

Re: pg_get_expr locking

[Tom Lane]

Peter Eisentraut  writes:
> On 07.02.24 16:26, Tom Lane wrote:
>> Why would a test be applying pg_get_expr to a table it doesn't
>> control?

> I think the situation is that one test (domain) runs pg_get_expr as part 
> of an information_schema view, while at the same time another test 
> (alter_table) drops a table that the pg_get_expr is just processing.

The test case that's failing is, IIUC,

+SELECT * FROM information_schema.domain_constraints
+  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+  ORDER BY constraint_name;

I see no use of pg_get_expr() in the domain_constraints view:

CREATE VIEW domain_constraints AS
SELECT CAST(current_database() AS sql_identifier) AS constraint_catalog,
   CAST(rs.nspname AS sql_identifier) AS constraint_schema,
   CAST(con.conname AS sql_identifier) AS constraint_name,
   CAST(current_database() AS sql_identifier) AS domain_catalog,
   CAST(n.nspname AS sql_identifier) AS domain_schema,
   CAST(t.typname AS sql_identifier) AS domain_name,
   CAST(CASE WHEN condeferrable THEN 'YES' ELSE 'NO' END
 AS yes_or_no) AS is_deferrable,
   CAST(CASE WHEN condeferred THEN 'YES' ELSE 'NO' END
 AS yes_or_no) AS initially_deferred
FROM pg_namespace rs, pg_namespace n, pg_constraint con, pg_type t
WHERE rs.oid = con.connamespace
  AND n.oid = t.typnamespace
  AND t.oid = con.contypid
  AND (pg_has_role(t.typowner, 'USAGE')
   OR has_type_privilege(t.oid, 'USAGE'));

I'm a little suspicious that the failure is actually coming from
somewhere down inside has_type_privilege(), but I traced through
that quickly and don't see how it could reach such an error.
In any case I thought we'd hardened all those functions in 403ac226d.
So I'm still pretty mystified.  Have you had any luck in making
the failure reproducible?

regards, tom lane

Re: Put genbki.pl output into src/include/catalog/ directly

[Tom Lane]

Peter Eisentraut  writes:
> With the makefile rules, the output of genbki.pl was written to
> src/backend/catalog/, and then the header files were linked to
> src/include/catalog/.

> This patch changes it so that the output files are written directly to
> src/include/catalog/.

Didn't read the patch, but +1 for concept.

regards, tom lane

Re: What about Perl autodie?

[Dagfinn Ilmari Mannsåker]

Daniel Gustafsson  writes:

>> On 8 Feb 2024, at 16:53, Tom Lane  wrote:
>
>> 2. Don't wait, migrate them all now.  This would mean requiring
>> Perl 5.10.1 or later to run the TAP tests, even in back branches.
>> 
>> I think #2 might not be all that radical.  We have nothing older
>> than 5.14.0 in the buildfarm, so we don't really have much grounds
>> for claiming that 5.8.3 will work today.  And 5.10.1 came out in
>> 2009, so how likely is it that anyone cares anymore?
>
> I would vote for this option, if we don't run the trailing edge anywhere where
> breakage is visible to developers then it is like you say, far from guaranteed
> to work.

The oldest Perl I'm aware of on a still-supported (fsvo) OS is RHEL 6,
which shipped 5.10.1 and has Extended Life-cycle Support until
2024-06-30.

For comparison, last year the at the Perl Toolchain Summit in Lyon we
decided that toolchain modules (the modules needed to build, test and
install CPAN distributions) are only required support versions of Perl
up to 10 years old, i.e. currently 5.18 (but there's a one-time
excemption to keep it to 5.16 until RHEL 7 goes out of maintenance
support on 2024-06-30).

- ilmari

Re: What about Perl autodie?

[Greg Sabino Mullane]

>
> 2. Don't wait, migrate them all now.  This would mean requiring
> Perl 5.10.1 or later to run the TAP tests, even in back branches.
>

#2 please. For context, meson did not even exist in 2009.

Cheers,
Greg

Re: What about Perl autodie?

[Daniel Gustafsson]

> On 8 Feb 2024, at 16:53, Tom Lane  wrote:

> 2. Don't wait, migrate them all now.  This would mean requiring
> Perl 5.10.1 or later to run the TAP tests, even in back branches.
> 
> I think #2 might not be all that radical.  We have nothing older
> than 5.14.0 in the buildfarm, so we don't really have much grounds
> for claiming that 5.8.3 will work today.  And 5.10.1 came out in
> 2009, so how likely is it that anyone cares anymore?

I would vote for this option, if we don't run the trailing edge anywhere where
breakage is visible to developers then it is like you say, far from guaranteed
to work.

--
Daniel Gustafsson

Re: What about Perl autodie?

[Tom Lane]

Daniel Gustafsson  writes:
>> On 8 Feb 2024, at 08:01, Peter Eisentraut  wrote:
>> I suppose we could start using it for completely new scripts.

> +1, it would be nice to eventually be able to move to it everywhere so 
> starting
> now with new scripts may make the eventual transition smoother.

I'm still concerned about people carelessly using autodie-reliant
code in places where they shouldn't.  I offer two safer ways
forward:

1. Wait till v16 is the oldest supported branch, and then migrate
both HEAD and back branches to using autodie.

2. Don't wait, migrate them all now.  This would mean requiring
Perl 5.10.1 or later to run the TAP tests, even in back branches.

I think #2 might not be all that radical.  We have nothing older
than 5.14.0 in the buildfarm, so we don't really have much grounds
for claiming that 5.8.3 will work today.  And 5.10.1 came out in
2009, so how likely is it that anyone cares anymore?

regards, tom lane

RE: Psql meta-command conninfo+

[Maiquel Grassi]

> On 07.02.24 21:13, Maiquel Grassi wrote:
>>
>> I believe in v7 patch we have a quite substantial meta-command feature.
>>
>>
> Thanks for implementing this. I find it very handy.
>
> I was just wondering if a "permission denied" error for non-superusers
> is the best choice for "\conninfo+":
>
> postgres=> \conninfo+
> ERROR:  permission denied to examine "unix_socket_directories"
> DETAIL:  Only roles with privileges of the "pg_read_all_settings" role
> may examine this parameter.
>
> .. since it is not the case with "\conninfo":
>
> postgres=> \conninfo
> You are connected to database "postgres" as user "jim" via socket in
> "/tmp" at port "5432".
>
> Perhaps excluding the column from the result set or returning NULL in
> the affected columns would be less confusing?
>
> There are also some indentation issues in your patch:
>
> /home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:142:
> indent with spaces.
> PGresult   *res;
> /home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:143:
> indent with spaces.
> PQExpBufferData buf;
> /home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:144:
> indent with spaces.
> printQueryOpt myopt = pset.popt;
> /home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:146:
> indent with spaces.
> initPQExpBuffer();
> /home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:148:
> indent with spaces.
> printfPQExpBuffer(,
> warning: squelched 34 whitespace errors
> warning: 39 lines add whitespace errors.
>
> Looking forward to see the documentation and tests!

--//--

Hi Jim,
Thank you for your support on this patch!
As I believe in its usability, I have been dedicating efforts to make it really 
interesting.
I hadn't thought about the permissioning issue for "unix_socket_directories". I 
appreciate that.
I thought about solving this situation using the same approach as \conninfo. I 
added the validation if (is_unixsock_path(host) && !(hostaddr && *hostaddr)) in 
the SQL part along with an "append". In case of a negative result, another 
"append" adds NULL.
Regarding the whitespace issue, before generating v8 patch file, I used 
pgindent to adjust each modified file. I believe it should be ok now. If you 
could verify, I'd be grateful.
Below are the tests after adjusting for the permissioning issues:

[postgres@localhost bin]$ ./psql
psql (17devel)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" via socket in 
"/tmp" at port "5432".
postgres=# \conninfo+

  Current Connection Information
 Database | Authenticated User | System User | Current User | Session User | 
Session PID | Server Version | Server Address | Server Port | Client Address | 
Client Port | Socket Directory | Host
--++-+--+--+-+++-++-+--+--
 postgres | postgres   | | postgres | postgres |
   31479 | 17devel|| 5432|| 
| /tmp |
(1 row)

postgres=# CREATE USER maiquel;
CREATE ROLE
postgres=# \q
[postgres@localhost bin]$ ./psql -U maiquel -d postgres
psql (17devel)
Type "help" for help.

postgres=> \conninfo
You are connected to database "postgres" as user "maiquel" via socket in "/tmp" 
at port "5432".
postgres=> \conninfo+

  Current Connection Information
 Database | Authenticated User | System User | Current User | Session User | 
Session PID | Server Version | Server Address | Server Port | Client Address | 
Client Port | Socket Directory | Host
--++-+--+--+-+++-++-+--+--
 postgres | maiquel| | maiquel  | maiquel  |
   31482 | 17devel|| 5432|| 
| /tmp |
(1 row)

postgres=> \q
[postgres@localhost bin]$ ./psql -h localhost -U maiquel -d postgres
psql (17devel)
Type "help" for help.

postgres=> \conninfo
You are connected to database "postgres" as user "maiquel" on host "localhost" 
(address "::1") at port "5432".
postgres=> \conninfo+

Current Connection Information
 Database | Authenticated User | System User | Current User | Session User | 
Session PID | Server Version | Server Address | Server Port | Client Address | 
Client Port | Socket Directory |   Host

Re: Sequence Access Methods, round two

[Peter Eisentraut]

On 19.01.24 00:27, Michael Paquier wrote:

The reason why this stuff has bumped into my desk is that we have no
good solution in-core for globally-distributed transactions for
active-active deployments.  First, anything we have needs to be
plugged into default expressions of attributes like with [1] or [2],
or a tweak is to use sequence values that are computed with different
increments to avoid value overlaps across nodes.  Both of these
require application changes, which is meh for a bunch of users.


I don't follow how these require "application changes".  I guess it 
depends on where you define the boundary of the "application".  The 
cited solutions require that you specify a different default expression 
for "id" columns.  Is that part of the application side?  How would your 
solution work on that level?  AFAICT, you'd still need to specify the 
sequence AM when you create the sequence or identity column.  So you'd 
need to modify the DDL code in any case.

RE: Psql meta-command conninfo+

[Maiquel Grassi]

> Hi,

> On 07.02.2024 23:13, Maiquel Grassi wrote:


> Regarding the "system_user" function, as it is relatively new, I added
> the necessary handling to avoid conflicts with versions lower than version 16.


> Yes, that's rights.
>
> A couple of doubts about the implementation details.
> But keep in mind that I'm not very good at programming in the C language.
> I hope for the help of more experienced developers.
>
>
> 1.
> + if (db == NULL)
> + printf(_("You are currently not connected to a 
> database.\n"));
>
> This check is performed for \conninfo, but not for \conninfo+.
>
>
> 2.
> Some values (address, socket) are evaluated separately for \conninfo
> (via C functions) and for \conninfo+ (via sql functions).
> It may be worth evaluating them in one place. But I'm not sure about that.
>
> The final version of the patch will require changes to the documentation and 
> tests.

--//--

Hi Pavel,

First of all, thank you very much for your observations.

1. The connection check for the case of \conninfo+ is handled by "describe.c" 
itself since it deals with queries. I might be mistaken, but I believe that by 
using "printQuery()" via "describe.c", this is already ensured, and there is no 
need to evaluate the connection status.

2. I believe that by implementing the evaluations separately as they are, it 
becomes easier to perform future maintenance by minimizing the mixing of C code 
with SQL code as much as possible. However, certainly, the possibility of a 
better suggestion than mine remains open.

Regards,
Maiquel O. Grassi.

Re: the s_lock_stuck on perform_spin_delay

[Andy Fan]

Hi, 

> There are some similarities between this and
> https://www.postgresql.org/message-id/20240207184050.rkvpuudq7huijmkb%40awork3.anarazel.de
> as described in that email.

Thanks for this information.

>
>
> Hm, I think this might make this code a bit more expensive. It's cheaper, both
> in the number of instructions and their cost, to set variables on the stack
> than in global memory - and it's already performance critical code.  I think
> we need to optimize the code so that we only do init_local_spin_delay() once
> we are actually spinning, rather than also on uncontended locks.

A great lession learnt, thanks for highlighting this!
>
>
>
>> @@ -5432,20 +5431,29 @@ LockBufHdr(BufferDesc *desc)
>>  static uint32
>>  WaitBufHdrUnlocked(BufferDesc *buf)
>>  {
>> -SpinDelayStatus delayStatus;
>>  uint32  buf_state;
>>
>> -init_local_spin_delay();
>> +/*
>> + * Suppose the buf will not be locked for a long time, setup a spin on
>> + * this.
>> + */
>> +init_local_spin_delay();
>
> I don't know what this comment really means.

Hmm, copy-paste error. Removed in v10.

>
>
>> +#ifdef USE_ASSERT_CHECKING
>> +void
>> +VerifyNoSpinLocksHeld(bool check_in_panic)
>> +{
>> +if (!check_in_panic && spinStatus.in_panic)
>> +return;
>
> Why do we need this?

We disallow errstart when a spin lock is held then there are two
speical cases need to be handled.

a). quickdie signal handler. The reason is explained with the below
comments. 

/*
 * It is possible that getting here when holding a spin lock already.
 * However current function needs some actions like elog which are
 * disallowed when holding a spin lock by spinlock misuse detection
 * system. So tell that system to treat this specially.
 */
spinStatus.in_panic = true;

b). VerifyNoSpinLocksHeld function.

if (spinStatus.func != NULL)
{
/*
 * Now we have held a spin lock and then errstart is disallow, 
 * to avoid the endless recursive call of VerifyNoSpinLocksHeld
 * because of the VerifyNoSpinLocksHeld checks in errstart,
 * set spinStatus.in_panic to true to break the cycle.
 */
spinStatus.in_panic = true;
elog(PANIC, "A spin lock has been held at %s:%d",
 spinStatus.func, spinStatus.line);
}

>
>
>> diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
>> index aa06e49da2..c3fe75a41d 100644
>> --- a/src/include/storage/s_lock.h
>> +++ b/src/include/storage/s_lock.h
>> @@ -652,7 +652,10 @@ tas(volatile slock_t *lock)
>>   */
>>  #if !defined(S_UNLOCK)
>>  #define S_UNLOCK(lock)  \
>> -do { __asm__ __volatile__("" : : : "memory");  *(lock) = 0; } while (0)
>> +do { __asm__ __volatile__("" : : : "memory"); \
>> +ResetSpinLockStatus(); \
>> +*(lock) = 0; \
>> +} while (0)
>>  #endif
>
> That seems like the wrong place. There are other definitions of S_UNLOCK(), so
> we clearly can't do this here.

True, in the v10, the spinlock_finish_release() is called in
SpinLockRelease. The side effect is if user calls S_UNLOCK directly,
there would be something wrong because lack of call
spinlock_finish_release, I found this usage in regress.c (I changed it
to SpinLockRelease). but I think it is OK because:

1) in s_lock.h, there is clear comment say:

 *  NOTE: none of the macros in this file are intended to be called 
directly.
 *  Call them through the hardware-independent macros in spin.h.

2). If someone breaks the above rule, the issue can be found easily in
assert build.

3). It has no impact on release build.

>>
>> -#define init_local_spin_delay(status) init_spin_delay(status, __FILE__, 
>> __LINE__, __func__)
>> -extern void perform_spin_delay(SpinDelayStatus *status);
>> -extern void finish_spin_delay(SpinDelayStatus *status);
>> +#define init_local_spin_delay() init_spin_delay( __FILE__, __LINE__, 
>> __func__)
>> +extern void perform_spin_delay(void);
>> +extern void finish_spin_delay(void);
>
> As an API this doesn't quite make sense to me. For one, right now an
> uncontended SpinLockAcquire afaict will not trigger this mechanism, as we
> never call init_spin_delay().

Another great lesssion learnt, thanks for this as well!

>
>
> Maybe we could have

I moved on with the below suggestion with some small modification.

>
> - spinlock_prepare_acquire() - about to acquire a spinlock
>   empty in optimized builds, asserts that no other spinlock is held
> etc
>
>   This would get called in SpinLockAcquire(), LockBufHdr() etc.

"asserts that no other spinlock" has much more user cases than
SpinLockAcquire / LockBufHdr, I think sharing the same function will be
OK which is VerifyNoSpinLocksHeld function for now. 


> - spinlock_finish_acquire() - have acquired spinlock
>   empty in optimized builds, in assert builds sets variable indicating we're
>   in spinlock
>
>   This would get called in SpinLockRelease() etc.

I think you mean "SpinLockAcquire" here.

Matthias 

Re: pg_stat_advisor extension

[Ilia Evdokimov]

On Feb 08 2024 at 07:14:18, Andrei Lepikhov wrote:

1. In the case of parallel workers the plan_rows value has a different 
semantics than the number of rows predicted. Just explore 
get_parallel_divisor().
Yes, this is a very weighty and important issue. I need to think about 
this very carefully.
2. The extension recommends new statistics immediately upon an error 
finding. But what if the reason for the error is stale statistics? Or 
this error may be raised for only one specific set of constants, and 
estimation will be done well in another 99.% of cases for the same 
expression.
According to No.2, it might make sense to collect and track clause 
combinations and cardinality errors found and let the DBA make decisions 
on their own.
Your proposal is very interesting. In my opinion, it is worth 
considering updating the extended statistics if they are truly stale. 
And write about this in a separate message with suggestion updating 
statistics.
If I succeed, then in the next patch I will add the kind of extended 
statistics to the message, deal with the parallel workers and update 
statistics if necessary.
If you have additional suggestions and thoughts, feel free to write them 
in this thread.



Regards,
Ilia Evdokimov,
Tantor Labs LLC.

Re: Reducing connection overhead in pg_upgrade compat check phase

[Daniel Gustafsson]

> On 8 Feb 2024, at 11:55, Peter Eisentraut  wrote:

> A few more quick comments:

Thanks for reviewing!

> I think the .report_text assignments also need a gettext_noop(), like the 
> .status assignments.

Done in the attached.

> The type DataTypesUsageChecks is only used in check.c, so doesn't need to be 
> in pg_upgrade.h.

Fixed.

> Idea for further improvement: Might be nice if the DataTypesUsageVersionCheck 
> struct also included the applicable version information, so the additional 
> checks in version.c would no longer be necessary.

I tried various variants of this when writing it, but since the checks aren't
just checking version but also include catalog version checks it became messy.
One option could perhaps be to include a version number for <= comparison, and
if set to zero a function pointer to a version check function must be provided?
That would handle the simple cases in a single place without messy logic, and
leave the more convoluted checks with a special case function.

--
Daniel Gustafsson



v12-0001-pg_upgrade-run-all-data-type-checks-per-connecti.patch
Description: Binary data

Re: pg_stat_advisor extension

[Ilia Evdokimov]

Our further discussion of this new extension takes place in this thread: https://www.postgresql.org/message-id/flat/f822b674-9697-43b9-931b-4d69729a26ff%40tantorlabs.com .Due to technical difficulties in the current thread, I will not be able to conduct a dialogue except in HTML format. And this will make it inconvenient for everyone to read the messages. I apologize for the inconvenience caused. Regards, Ilia Evdokimov, TantorLabs LLC.

Re: 2024-02-08 release announcement draft

[Dagfinn Ilmari Mannsåker]

jian he  writes:

> On Thu, Feb 8, 2024 at 1:17 PM Tom Lane  wrote:
>>
>> "Jonathan S. Katz"  writes:
>> > On 2/6/24 3:19 AM, jian he wrote:
>> >> On Tue, Feb 6, 2024 at 12:43 PM Jonathan S. Katz  
>> >> wrote:
>> >>> * In PL/pgSQL, support SQL commands that are `CREATE FUNCTION`/`CREATE
>> >>> PROCEDURE` with SQL-standard bodies.
>>
>> >> https://git.postgresql.org/cgit/postgresql.git/commit/?id=57b440ec115f57ff6e6a3f0df26063822e3123d2
>> >> says only for plpgsql routine or DO block.
>>
>> > I had some trouble wordsmithing this, but the commit title is pretty
>> > clear to me so I opted for that.
>>
>> Your text seems fine to me.  I'm confused about the objection here:
>> exactly what uses of plpgsql aren't in a routine or DO block?
>>
>> regards, tom lane
>
> I guess I was confused with cases like this
> `
> create function test1() returns int language plpgsql
> begin atomic
> select unique1 from tenk1 limit 1;
> end ;
> `
> looking back, the original text seems fine.

The word "routine" is the SQL standard's common term for both functions
and procedures.

- ilmari

Re: pg_stat_advisor extension

[Ilia Evdokimov]

On Feb 8 2024 at 00:00:00 jian he

>INT MAX

>should be 1.0?

I don’t know why Konstantin Knizhnik used the ratio of actual tuples to 
the planned ones, but most who start testing my extension expect that it 
will be a coefficient from 0 to 1, which will be the ratio of the 
estimated tuples to the actual ones. Therefore, I changed the value of 
this coefficient the other way around and now the value can be from 0 to 
1. The patch with changes has been attached.



> now CREATE STATISTICS, the statistics name is optional

I constructed the name of the statistics so that the user could copy the 
line with 'CREATE STATISTICS' with the mouse and execute this command 
faster. But if the user wants ITS name, he can do it manually.



> here you can explicitly mention the statistics kind would be great

I agree with you. That would be my next step. That's why I'm doing it now.


> Also since the documentation is limited, more comments 
explainingSuggestMultiColumnStatisticsForNode would be great.



overall the comments are very little, it should be more (that's my opinion).


Yes, certainly. I'll do it in the next patch.

I'm looking forward to your thoughts and feedback.

Regards,

Ilia Evdokimov,

Tantor Labs LLC.
From f87f4a57e532d57f43dab4764d08ddf83d9f3d8f Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov 
Date: Thu, 8 Feb 2024 16:00:57 +0300
Subject: [PATCH] 'pg_stat_advisor' extension.

This serves as a hook into the executor for determining total rows and planned rows.
The process starts by checking the `pg_stat_advisor.suggest_statistics_threshold` GUC parameter.
If it's set to 0.0, the extension does not proceed further. When the parameter is greater than 0.0,
the extension evaluates the accuracy of planned rows. A suggestion for creating statistics is made
if the ratio of total rows to planned rows is greater than or equal to this threshold.

Only then does it extract the relation and columns from the query.
The extension checks pg_statistic_ext for existing relevant statistics. If no statistics are found,
it prints a notice suggesting the creation of statistics, using the naming format 'relationName_columns'.

Author: Ilia Evdokimov
---
 contrib/Makefile  |   1 +
 contrib/meson.build   |   1 +
 contrib/pg_stat_advisor/.gitignore|   3 +
 contrib/pg_stat_advisor/Makefile  |  20 +
 contrib/pg_stat_advisor/README.md |  85 +++
 .../expected/pg_stat_advisor.out  |  96 
 contrib/pg_stat_advisor/meson.build   |  30 ++
 contrib/pg_stat_advisor/pg_stat_advisor.c | 482 ++
 .../pg_stat_advisor/sql/pg_stat_advisor.sql   |  50 ++
 9 files changed, 768 insertions(+)
 create mode 100644 contrib/pg_stat_advisor/.gitignore
 create mode 100644 contrib/pg_stat_advisor/Makefile
 create mode 100644 contrib/pg_stat_advisor/README.md
 create mode 100644 contrib/pg_stat_advisor/expected/pg_stat_advisor.out
 create mode 100644 contrib/pg_stat_advisor/meson.build
 create mode 100644 contrib/pg_stat_advisor/pg_stat_advisor.c
 create mode 100644 contrib/pg_stat_advisor/sql/pg_stat_advisor.sql

diff --git a/contrib/Makefile b/contrib/Makefile
index da4e2316a3..da9a4ceeaa 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -34,6 +34,7 @@ SUBDIRS = \
 		pg_buffercache	\
 		pg_freespacemap \
 		pg_prewarm	\
+		pg_stat_advisor \
 		pg_stat_statements \
 		pg_surgery	\
 		pg_trgm		\
diff --git a/contrib/meson.build b/contrib/meson.build
index c12dc906ca..a20d99443b 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -49,6 +49,7 @@ subdir('pgcrypto')
 subdir('pg_freespacemap')
 subdir('pg_prewarm')
 subdir('pgrowlocks')
+subdir('pg_stat_advisor')
 subdir('pg_stat_statements')
 subdir('pgstattuple')
 subdir('pg_surgery')
diff --git a/contrib/pg_stat_advisor/.gitignore b/contrib/pg_stat_advisor/.gitignore
new file mode 100644
index 00..913175ff6e
--- /dev/null
+++ b/contrib/pg_stat_advisor/.gitignore
@@ -0,0 +1,3 @@
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_stat_advisor/Makefile b/contrib/pg_stat_advisor/Makefile
new file mode 100644
index 00..f31b939e8a
--- /dev/null
+++ b/contrib/pg_stat_advisor/Makefile
@@ -0,0 +1,20 @@
+# contrib/pg_stat_advisor/Makefile
+
+MODULE_big = pg_stat_advisor
+OBJS = \
+	$(WIN32RES) \
+	pg_stat_advisor.o
+PGFILEDESC = "pg_stat_advisor - analyze query performance and recommend the creation of additional statistics"
+
+REGRESS = pg_stat_advisor
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/pg_stat_advisor
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/pg_stat_advisor/README.md b/contrib/pg_stat_advisor/README.md
new file mode 100644
index 00..3f4e97f195
--- /dev/null
+++ b/contrib/pg_stat_advisor/README.md
@@ -0,0 +1,85 @@
+## pg_stat_advisor - PostgreSQL advisor 

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[James Coleman]

On Thu, Feb 8, 2024 at 4:47 AM Ashutosh Bapat
 wrote:
>
> On Thu, Feb 8, 2024 at 9:57 AM Laurenz Albe  wrote:
> >
> > On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > > -   how to set the replica identity.  If a table without a replica 
> > > identity is
> > > +   how to set the replica identity.  If a table without a replica 
> > > identity
> > > +   (or with replica identity behavior the same as 
> > > NOTHING) is
> > > added to a publication that replicates UPDATE
> > > or DELETE operations then
> > > subsequent UPDATE or DELETE
> >
> > I had the impression that the root of the confusion was the perceived 
> > difference
> > between "REPLICA IDENTITY NOTHING" and "no replica identity", and that 
> > change
> > doesn't improve that.
> >
> > How about:
> >
> >   If a table without a replica identity (explicitly set to 
> > NOTHING,
> >   or set to a primary key or index that doesn't exist) is added ...
>
> Another possibility is just to improve the documentation of various
> options as follows.
>
> DEFAULT
>
> If there is a primary key, record the old values of the columns of the
> primary key. Otherwise it acts as NOTHING. This is the default for
> non-system tables.
>
> USING INDEX index_name
>
> Records the old values of the columns covered by the named index, that
> must be unique, not partial, not deferrable, and include only columns
> marked NOT NULL. If this index is dropped, the behavior is the same as
> NOTHING.
>
> FULL
>
> Records the old values of all columns in the row.
>
> NOTHING
>
> Records no information about the old row. This is equivalent to having
> no replica identity. This is the default for system tables.

This is the simplest change, and it does solve the confusion, so I'd
be happy with it also. The other proposals have the benefit of having
all the information necessary on the publications page rather than
requiring the user to refer to the ALTER TABLE REPLICA IDENTITY page
to understand what's meant.

Regards,
James Coleman

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[James Coleman]

On Wed, Feb 7, 2024 at 11:27 PM Laurenz Albe  wrote:
>
> On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > -   how to set the replica identity.  If a table without a replica identity 
> > is
> > +   how to set the replica identity.  If a table without a replica identity
> > +   (or with replica identity behavior the same as 
> > NOTHING) is
> > added to a publication that replicates UPDATE
> > or DELETE operations then
> > subsequent UPDATE or DELETE
>
> I had the impression that the root of the confusion was the perceived 
> difference
> between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
> doesn't improve that.
>
> How about:
>
>   If a table without a replica identity (explicitly set to 
> NOTHING,
>   or set to a primary key or index that doesn't exist) is added ...

I think that would work also. I was reading the initial suggestion as
"(or with replica identity behavior the same as..." as defining what
"without a replica identity" meant, which would avoid the confusion.
But your proposal is more explicit and more succinct, so I think it's
the better option of the two.

Regards,
James Coleman

Re: Thoughts about NUM_BUFFER_PARTITIONS

[wenhui qiu]

Hi Heikki Linnakangas
I think the larger shared buffer  higher the probability of multiple
backend processes accessing the same bucket slot BufMappingLock
simultaneously, （   InitBufTable(NBuffers + NUM_BUFFER_PARTITIONS); When I
have free time, I want to do this test. I have seen some tests, but the
result report is in Chinese


Best wishes

Heikki Linnakangas  于2024年2月8日周四 19:26写道：

> On 08/02/2024 12:17, wenhui qiu wrote:
> > HI hackers
> >  When I read this text in this document there is a paragraph in
> > it(https://www.interdb.jp/pg/pgsql08/03.html
> > )
> > /*
> > The BufMappingLock is split into partitions to reduce contention in the
> > buffer table (the default is 128 partitions). Each BufMappingLock
> > partition guards a portion of the corresponding hash bucket slots.
> > */,
> >
> > Physical servers with terabytes of RAM are now commonplace,I'm looking
> > at the comments inside the source code.I'm looking at the comments
> > inside the source code to see if they still match the current hardware
> > capability?
>
> The optimal number of partitions has more to do with the number of
> concurrent processes using the buffer cache, rather than the size of the
> cache. The number of CPUs in servers has increased too, but not as much
> as RAM.
>
> But yeah, it's a valid question if the settings still make sense with
> modern hardware.
>
> > The  comment says that in the future there may be a
> > parameter,Iam a  dba now and I try to think of implementing this
> > parameter, but I'm not a professional kernel developer, I still want the
> > community senior developer to implement this parameter
>
> The first thing to do would be to benchmark with different
> NUM_BUFFER_PARTITIONS settings, and see if there's benefit in having
> more partitions.
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>
>

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 3:56 AM Nathan Bossart 
wrote:

> On Thu, Feb 08, 2024 at 03:49:03PM +1300, Thomas Munro wrote:
> > On Thu, Feb 8, 2024 at 3:38 PM Thomas Munro 
> wrote:
> >> Perhaps you could wrap it in a branch-free sign() function so you get
> >> a narrow answer?
> >>
> >> https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c
> >
> > Ah, strike that, it is much like the suggested (a > b) - (a < b) but
> > with extra steps...
>
> Yeah, https://godbolt.org/ indicates that the sign approach compiles to
>
> movsx   rsi, esi
> movsx   rdi, edi
> xor eax, eax
> sub rdi, rsi
> testrdi, rdi
> setgal
> shr rdi, 63
> sub eax, edi
> ret
>
> while the approach Andres suggested compiles to
>
> xor eax, eax
> cmp edi, esi
> setldl
> setgal
> movzx   edx, dl
> sub eax, edx
> ret
>

Here is a patch that fixes existing cases and introduces a macro for this
comparison (it uses the (a > b) - (a < b) approach). Not sure where to
place the macro nor what a suitable name should be, so feel free to suggest
anything.

I also noted that some functions are duplicated and it might be an idea to
introduce a few standard functions like pg_qsort_strcmp for, e.g., integers
and other common types.

Also noted it is quite common to have this pattern in various places to do
lexicographic sort of multiple values and continue the comparison if they
are equal. Not sure if that is something we should look at.

Best wishes,
Mats Kindahl

>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>
From 8c183806a6b0f95ab53db4b029bc82823785c363 Mon Sep 17 00:00:00 2001
From: Mats Kindahl 
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: Standardize integer comparison for qsort()

Introduce a macro INT_CMP() that will standardize integer comparison
for implementing comparison function for qsort(). The macro will ensure
that the function returns -1, 0, or 1 without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.
---
 contrib/hstore/hstore_gist.c|  2 +-
 contrib/intarray/_intbig_gist.c |  2 +-
 contrib/pg_stat_statements/pg_stat_statements.c |  7 +--
 contrib/pg_trgm/trgm_op.c   |  7 +--
 contrib/seg/seg.c   |  7 +--
 src/backend/access/nbtree/nbtinsert.c   |  7 +--
 src/backend/access/nbtree/nbtpage.c |  9 ++---
 src/backend/access/nbtree/nbtsplitloc.c |  7 +--
 src/backend/access/spgist/spgdoinsert.c |  4 +---
 src/backend/access/spgist/spgkdtreeproc.c   |  8 ++--
 src/backend/access/spgist/spgtextproc.c |  2 +-
 src/backend/backup/basebackup_incremental.c |  7 +--
 src/backend/backup/walsummary.c |  6 +-
 src/backend/catalog/heap.c  |  7 +--
 src/backend/nodes/list.c| 12 ++--
 src/backend/nodes/tidbitmap.c   |  6 +-
 src/backend/optimizer/geqo/geqo_pool.c  |  7 +--
 src/backend/parser/parse_agg.c  |  2 +-
 src/backend/postmaster/autovacuum.c |  5 +
 src/backend/replication/logical/reorderbuffer.c |  6 +-
 src/backend/replication/syncrep.c   |  7 +--
 src/backend/utils/adt/oid.c |  6 +-
 src/backend/utils/adt/tsgistidx.c   |  9 ++---
 src/backend/utils/adt/tsquery_gist.c|  5 +
 src/backend/utils/adt/tsvector.c|  4 +---
 src/backend/utils/adt/tsvector_op.c |  4 +---
 src/backend/utils/adt/xid.c |  6 +-
 src/backend/utils/cache/relcache.c  |  2 +-
 src/backend/utils/cache/syscache.c  |  4 +---
 src/backend/utils/cache/typcache.c  |  7 +--
 src/backend/utils/resowner/resowner.c   |  9 +
 src/bin/pg_dump/pg_dump_sort.c  |  6 +-
 src/bin/pg_upgrade/function.c   |  8 
 src/bin/pg_walsummary/pg_walsummary.c   |  7 +--
 src/bin/psql/crosstabview.c |  2 +-
 src/include/access/gin_private.h|  7 +--
 src/include/c.h |  8 
 37 files changed, 51 insertions(+), 170 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..e125b76290 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -356,7 +356,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_intbig_gist.c 

Re: Simplify documentation related to Windows builds

[Nazir Bilal Yavuz]

Hi,

On Tue, 30 Jan 2024 at 11:02, Michael Paquier  wrote:
>
> On Fri, Jan 19, 2024 at 06:11:40AM -0500, Andrew Dunstan wrote:
> > FYI Strawberry was a bit stuck for a while at 5.32, but they are now up to
> > 5.38. See 
> >
> > I agree we shouldn't be recommending any particular perl distro, especially
> > not ASPerl which now has annoying license issues.
>
> The more I think about this thread, the more I'd tend to wipe out most
> of "windows-requirements" for the sole reason that it is the far-west
> regarding the various ways it is possible to get the dependencies we
> need for the build and at runtime.  We could keep it minimal with the
> set of requirements we are listing under meson in terms of versions:
> https://www.postgresql.org/docs/devel/install-requirements.html
>
> Then we could have one sentence recommending one, at most two
> facilities used the buildfarm, like https://www.msys2.org/ or
> chocolatey as these group basically all the dependencies we need for a
> meson build (right?) while linking back to the meson page about the
> version requirements.

I tested both msys2 and chocolatey on the fresh Windows containers and
I confirm that Postgres can be built using these. I tested the
dependencies that are required to build and run Postgres. If more
dependencies are required to be checked, I can test again.

As these will be continuously tested by the buildfarm, I agree that
what you suggested looks better.

> One issue I have with the meson page listing the requirements is that
> we don't directly mention Diff, but that's critical for the tests.

I think that is because most distros come with a preinstalled
diffutils package. It is mentioned under the Windows requirements page
[1] since it does not come preinstalled. However, I agree that it
could be good to mention it under the meson page listing the
requirements.

[1] 
https://www.postgresql.org/docs/devel/installation-platform-notes.html#WINDOWS-REQUIREMENTS

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Re: "ERROR: latch already owned" on gharial

[Heikki Linnakangas]

On 08/02/2024 04:08, Soumyadeep Chakraborty wrote:

A possible ordering of events:

(1) DisownLatch() is called by pid Y during ProcKill() and the write for
latch->owner_pid = 0 is NOT yet flushed to shmem.

(2) The PGPROC object for pid Y is returned to the free list.

(3) Pid X sees the same PGPROC object on the free list and grabs it.

(4) Pid X does sanity check inside OwnLatch during InitProcess and
still sees the
old value of latch->owner_pid = Y (and not = 0), and trips the ERROR.

The above sequence of operations should apply to PG HEAD as well.

Suggestion:

Should we do a pg_memory_barrier() at the end of DisownLatch(), like in
ResetLatch(), like the one introduced in [3]? This would ensure that the write
latch->owner_pid = 0; is flushed to shmem. The attached patch does this.


Hmm, there is a pair of SpinLockAcquire() and SpinLockRelease() in 
ProcKill(), before step 3 can happen. Comment in spin.h about 
SpinLockAcquire/Release:



 *  Load and store operations in calling code are guaranteed not to be
 *  reordered with respect to these operations, because they include a
 *  compiler barrier.  (Before PostgreSQL 9.5, callers needed to use a
 *  volatile qualifier to access data protected by spinlocks.)


That talks about a compiler barrier, though, not a memory barrier. But 
looking at the implementations in s_lock.h, I believe they do act as 
memory barrier, too.


So you might indeed have that problem on 9.4, but AFAICS not on later 
versions.


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: 2024-02-08 release announcement draft

[jian he]

On Thu, Feb 8, 2024 at 1:17 PM Tom Lane  wrote:
>
> "Jonathan S. Katz"  writes:
> > On 2/6/24 3:19 AM, jian he wrote:
> >> On Tue, Feb 6, 2024 at 12:43 PM Jonathan S. Katz  
> >> wrote:
> >>> * In PL/pgSQL, support SQL commands that are `CREATE FUNCTION`/`CREATE
> >>> PROCEDURE` with SQL-standard bodies.
>
> >> https://git.postgresql.org/cgit/postgresql.git/commit/?id=57b440ec115f57ff6e6a3f0df26063822e3123d2
> >> says only for plpgsql routine or DO block.
>
> > I had some trouble wordsmithing this, but the commit title is pretty
> > clear to me so I opted for that.
>
> Your text seems fine to me.  I'm confused about the objection here:
> exactly what uses of plpgsql aren't in a routine or DO block?
>
> regards, tom lane

I guess I was confused with cases like this
`
create function test1() returns int language plpgsql
begin atomic
select unique1 from tenk1 limit 1;
end ;
`
looking back, the original text seems fine.

Re: Catalog domain not-null constraints

[jian he]

On Wed, Feb 7, 2024 at 4:11 PM Peter Eisentraut  wrote:
>
> >
> > Interesting.  I couldn't reproduce this locally, even across different
> > operating systems.  The cfbot failures appear to be sporadic, but also
> > happening across multiple systems, so it's clearly not just a local
> > environment failure.  Can anyone else perhaps reproduce this locally?
>
> This patch set needed a rebase, so here it is.
>
do you think
add following
ALTER DOMAIN name ADD NOT
NULL VALUE

to doc/src/sgml/ref/alter_domain.sgml synopsis makes sense?
otherwise it would be hard to find out this command, i think.


I think I found a bug.
connotnull already set to not null.
every execution of  `alter domain connotnull add not null value ;`
would concatenate 'NOT NULL VALUE' for the "Check" column,
That means changes in the function pg_get_constraintdef_worker are not
100% correct.
see below demo:


src8=# \dD+
  List of domains
 Schema |Name|  Type   | Collation | Nullable | Default |
Check  | Access privileges | Description
++-+---+--+-++---+-
 public | connotnull | integer |   |  | | NOT
NULL VALUE |   |
 public | nnint  | integer |   | not null | | NOT
NULL VALUE |   |
(2 rows)

src8=# alter domain connotnull add not null value ;
ALTER DOMAIN
src8=# \dD+
 List of domains
 Schema |Name|  Type   | Collation | Nullable | Default |
   Check | Access privileges | Descript
ion
++-+---+--+-+---+---+-

 public | connotnull | integer |   | not null | | NOT
NULL VALUE NOT NULL VALUE |   |
 public | nnint  | integer |   | not null | | NOT
NULL VALUE|   |
(2 rows)

src8=# alter domain connotnull add not null value ;
ALTER DOMAIN
src8=# \dD+
 List of domains
 Schema |Name|  Type   | Collation | Nullable | Default |
  Check | Access privil
eges | Description
++-+---+--+-+--+--
-+-
 public | connotnull | integer |   | not null | | NOT
NULL VALUE NOT NULL VALUE NOT NULL VALUE |
 |
 public | nnint  | integer |   | not null | | NOT
NULL VALUE   |
 |
(2 rows)

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 12:01 PM Mats Kindahl  wrote:

> On Wed, Feb 7, 2024 at 9:56 PM Nathan Bossart 
> wrote:
>
>> On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote:
>> > Doesn't hurt to fix the comparison functions, and +1 on using the same
>> > pattern everywhere.
>>
>> I attached a new version of the patch with some small adjustments.  I
>> haven't looked through all in-tree qsort() comparators to see if any
>> others
>> need to be adjusted, but we should definitely do so as part of this
>> thread.
>> Mats, are you able to do this?
>>
>
> Sure, I checked them and the only ones remaining are those using int16.
> Shall I modify those as well?
>

Seems your additional patch dealt with at least one of the cases.


>
>
>> > However, we use our qsort() with user-defined comparison functions, and
>> we
>> > cannot make any guarantees about what they might do. So we must ensure
>> that
>> > our qsort() doesn't overflow, no matter what the comparison function
>> does.
>> >
>> > Looking at our ST_SORT(), it seems safe to me.
>>
>> Cool.
>>
>> --
>> Nathan Bossart
>> Amazon Web Services: https://aws.amazon.com
>>
>

Re: Thoughts about NUM_BUFFER_PARTITIONS

[Heikki Linnakangas]

On 08/02/2024 12:17, wenhui qiu wrote:

HI hackers
     When I read this text in this document there is a paragraph in 
it(https://www.interdb.jp/pg/pgsql08/03.html 
)

/*
The BufMappingLock is split into partitions to reduce contention in the 
buffer table (the default is 128 partitions). Each BufMappingLock 
partition guards a portion of the corresponding hash bucket slots.

*/,

Physical servers with terabytes of RAM are now commonplace,I'm looking 
at the comments inside the source code.I'm looking at the comments 
inside the source code to see if they still match the current hardware 
capability?


The optimal number of partitions has more to do with the number of 
concurrent processes using the buffer cache, rather than the size of the 
cache. The number of CPUs in servers has increased too, but not as much 
as RAM.


But yeah, it's a valid question if the settings still make sense with 
modern hardware.


The  comment says that in the future there may be a 
parameter,Iam a  dba now and I try to think of implementing this 
parameter, but I'm not a professional kernel developer, I still want the 
community senior developer to implement this parameter


The first thing to do would be to benchmark with different 
NUM_BUFFER_PARTITIONS settings, and see if there's benefit in having 
more partitions.


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: Refactoring backend fork+exec code

[Heikki Linnakangas]

On 07/02/2024 20:25, Andres Freund wrote:

On 2024-01-30 02:08:36 +0200, Heikki Linnakangas wrote:

 From 54f22231bb2540fc5957c14005956161e6fc9dac Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Wed, 24 Jan 2024 23:15:55 +0200
Subject: [PATCH v8 1/5] Remove superfluous 'pgprocno' field from PGPROC

It was always just the index of the PGPROC entry from the beginning of
the proc array. Introduce a macro to compute it from the pointer
instead.


Hm. The pointer math here is bit more expensive than in some other cases, as
the struct is fairly large and sizeof(PGPROC) isn't a power of two. Adding
more math into loops like in TransactionGroupUpdateXidStatus() might end up
showing up.


I added a MyProcNumber global variable that is set to 
GetNumberFromPGProc(MyProc). I'm not really concerned about the extra 
math, but with MyProcNumber it should definitely not be an issue. The 
few GetNumberFromPGProc() invocations that remain are in less 
performance-critical paths.


(In later patch, I switch backend ids to 0-based indexing, which 
replaces MyProcNumber references with MyBackendId)



Is this really related to the rest of the series?


It's not strictly necessary, but it felt prudent to remove it now, since 
I'm removing the backendID field too.



You can now convert a backend ID into an index into the PGPROC array
simply by subtracting 1. We still use 0-based "pgprocnos" in various
places, for indexes into the PGPROC array, but the only difference now
is that backend IDs start at 1 while pgprocnos start at 0.


Why aren't we using 0-based indexing for both? InvalidBackendId is -1, so
there'd not be a conflict, right?


Correct. I was being conservative and didn't dare to change the old 
convention. The backend ids are visible in a few places like "pg_temp_0" 
schema names, and pg_stat_get_*() functions.


One alternative would be to reserve and waste allProcs[0]. Then pgprocno 
and backend ID could both be direct indexes to the array, but 0 would 
not be used.


If we switch to 0-based indexing, it begs the question: why don't we 
merge the concepts of "pgprocno" and "BackendId" completely and call it 
the same thing everywhere? It probably would be best in the long run. It 
feels like a lot of churn though.


Anyway, I switched to 0-based indexing in the attached new version, to 
see what it looks like.



@@ -457,7 +442,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId 
xid, const char *gid,
Assert(LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE));

Assert(gxact != NULL);
-   proc = >allProcs[gxact->pgprocno];
+   proc = GetPGProcByNumber(gxact->pgprocno);

/* Initialize the PGPROC entry */
MemSet(proc, 0, sizeof(PGPROC));


This set of changes is independent of this commit, isn't it?


Yes. It's just for symmetry, now that we use GetNumberFromPGProc() to 
get the pgprocno.



diff --git a/src/backend/postmaster/auxprocess.c 
b/src/backend/postmaster/auxprocess.c
index ab86e802f21..39171fea06b 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -107,17 +107,7 @@ AuxiliaryProcessMain(AuxProcType auxtype)

BaseInit();

-   /*
-* Assign the ProcSignalSlot for an auxiliary process.  Since it doesn't
-* have a BackendId, the slot is statically allocated based on the
-* auxiliary process type (MyAuxProcType).  Backends use slots indexed 
in
-* the range from 1 to MaxBackends (inclusive), so we use MaxBackends +
-* AuxProcType + 1 as the index of the slot for an auxiliary process.
-*
-* This will need rethinking if we ever want more than one of a 
particular
-* auxiliary process type.
-*/
-   ProcSignalInit(MaxBackends + MyAuxProcType + 1);
+   ProcSignalInit();


Now that we don't need the offset here, we could move ProcSignalInit() into
BsaeInit() I think?


Hmm, doesn't feel right to me. BaseInit() is mostly concerned with 
setting up backend-private structures, and it's also called for a 
standalone backend.


I feel the process initialization codepaths could use some cleanup in 
general. Not sure what exactly.



+/*
+ * BackendIdGetProc -- get a backend's PGPROC given its backend ID
+ *
+ * The result may be out of date arbitrarily quickly, so the caller
+ * must be careful about how this information is used.  NULL is
+ * returned if the backend is not active.
+ */
+PGPROC *
+BackendIdGetProc(int backendID)
+{
+   PGPROC *result;
+
+   if (backendID < 1 || backendID > ProcGlobal->allProcCount)
+   return NULL;


Hm, doesn't calling BackendIdGetProc() with these values a bug? That's not
about being out of date or such.


Perhaps. I just followed the example of the old implementation, which 
also returns NULL on bogus inputs.



+/*
+ * BackendIdGetTransactionIds -- get a backend's transaction status
+ *
+ * Get the xid, xmin, nsubxid and overflow status of the backend.  The
+ * 

Re: pg_get_expr locking

[Peter Eisentraut]

On 07.02.24 16:26, Tom Lane wrote:

What workaround should we use if there are conflicts created by
concurrent regression tests?  Just move the tests around a bit until the
issue goes away?


Why would a test be applying pg_get_expr to a table it doesn't
control?


I think the situation is that one test (domain) runs pg_get_expr as part 
of an information_schema view, while at the same time another test 
(alter_table) drops a table that the pg_get_expr is just processing.

Re: GUC-ify walsender MAX_SEND_SIZE constant

[Majid Garoosi]

Thank you very much for your review.

I generally agree with your suggestions, so just applied them.
You can find the new patch in the attached file.

Best
Majid

On Tue, 6 Feb 2024 at 09:26, Michael Paquier  wrote:

> On Fri, Jan 19, 2024 at 11:04:50PM +0330, Majid Garoosi wrote:
> > However, this value does not need to be larger than wal_segment_size,
> > thus its checker function returns false if a larger value is set for
> > this.
> >
> > This is my first patch. So, I hope I haven't done something wrong. :'D
>
> You've done nothing wrong.  Thanks for the patch!
>
> +   if (*newval > wal_segment_size)
> +   return false;
> +   return true;
>
> I was not sure first that we need a dynamic check, but I could get why
> somebody may want to make it higher than 1MB these days.
>
> The patch is missing a couple of things:
> - Documentation in doc/src/sgml/config.sgml, that has a section for
> "Sending Servers".
> - It is not listed in postgresql.conf.sample.  I would suggest to put
> it in REPLICATION -> Sending Servers.
> The default value of 128kB should be mentioned in both cases.
>
> - * We don't have a good idea of what a good value would be; there's some
> - * overhead per message in both walsender and walreceiver, but on the
> other
> - * hand sending large batches makes walsender less responsive to signals
> - * because signals are checked only between messages.  128kB (with
> - * default 8k blocks) seems like a reasonable guess for now.
> [...]
> +   gettext_noop("Walsender procedure consists of a loop, reading
> wal_sender_max_send_size "
> + "bytes of WALs from disk and sending them to the receiver.
> Sending large "
> + "batches makes walsender less responsive to signals."),
>
> This is removing some information about why it may be a bad idea to
> use a too low value (message overhead) and why it may be a bad idea to
> use a too large value (responsiveness).  I would suggest to remove the
> second gettext_noop() in guc_tables.c and move all this information to
> config.sgml with the description of the new GUC.  Perhaps just put it
> after wal_sender_timeout in the sample file and the docs?
>
> Three comments in walsender.c still mention MAX_SEND_SIZE.  These
> should be switched to mention the GUC instead.
>
> I am switching the patch as waiting on author for now.
> --
> Michael
>
From fdd833b8b5cfdc1645f8288b27e9a92fc46f7951 Mon Sep 17 00:00:00 2001
From: Majid Garoosi 
Date: Fri, 19 Jan 2024 22:48:23 +0330
Subject: [PATCH v2] Add documentation for wal_sender_max_send_size

Detailed changes:
- Add documentation to postgresql's config docs
- Remove code comments mentioning the setting's old constant name
- Add the parameter to postgresql.conf.sample
---
 doc/src/sgml/config.sgml  | 26 ++
 src/backend/replication/walsender.c   | 27 +++
 src/backend/utils/init/postinit.c | 11 
 src/backend/utils/misc/guc_tables.c   | 11 
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/replication/walsender.h   |  1 +
 src/include/utils/guc_hooks.h |  1 +
 7 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 61038472c5..86b9488cd1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4405,6 +4405,32 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
   
  
 
+ 
+  wal_sender_max_send_size (integer)
+  
+   wal_sender_max_send_size configuration parameter
+  
+  
+  
+   
+Maximum size of chunks that wal sender reads from disk and sends to the
+standby servers in each iteration. Its default value is 16
+times XLOG_BLCKSZ, i.e. it is 128kB if
+the project is built with the default XLOG_BLCKSZ
+configuration parameter. This parameter can be changed without restarting the
+server.
+   
+   
+The minimum allowed value is XLOG_BLCKSZ bytes, typically
+8kB. Its maximum allowed value is
+wal_segment_size, typically 16MB.
+While a large value for this parameter reduces the responsiveness of WAL
+sender processes to the signals, setting small values increases the overhead
+of the messages both in WAL senders and WAL receivers.
+   
+  
+ 
+
  
   track_commit_timestamp (boolean)
   
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 087031e9dc..c429facded 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -96,17 +96,6 @@
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
 
-/*
- * Maximum data payload in a WAL data message.  Must be >= XLOG_BLCKSZ.
- *
- * We don't have a good idea of what a good value would be; there's some
- * overhead per message in both walsender and walreceiver, but on the other
- 

Re: Synchronizing slots from primary to standby

[shveta malik]

On Thu, Feb 8, 2024 at 4:31 PM shveta malik  wrote:
>
> On Thu, Feb 8, 2024 at 12:08 PM Peter Smith  wrote:
> >
> > Here are some review comments for patch v80_2-0001.
>
> Thanks for the feedback Peter. Addressed the comments in v81.

Missed to mention, Hou-san helped in addressing some of these comments
in v81.Thanks Hou-San.

thanks
Shveta

Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs

[Jakub Wartak]

Hi Daniel,


On Tue, Jan 30, 2024 at 3:29 PM Daniel Verite  wrote:

> PFA a rebased version.

Thanks for the patch! I've tested it using my original reproducer and
it works great now against the original problem description. I've
taken a quick look at the patch, it looks good for me. I've tested
using -Werror for both gcc 10.2 and clang 11.0 and it was clean. I
have one slight doubt:

when I run with default pager (more or less):
\set FETCH_COUNT 1000
WITH data AS (SELECT  generate_series(1, 2000) as Total) select
repeat('a',100) || data.Total || repeat('b', 800) as total_pat from
data;
-- it enters pager, a skip couple of pages and then "q"

.. then - both backend and psql - go into 100% CPU as it were still
receiving (that doesn't happen e.g. with export PAGER=cat).  So I'm
not sure, maybe ExecQueryAndProcessResults() should somewhat faster
abort when the $PAGER is exiting normally(?).

And oh , btw, in v6-0001 (so if you would be sending v7 for any other
reason -- other reviewers -- maybe worth realigning it as detail):

+  int PQsetChunkedRowsMode(PGconn *conn,
+   int maxRows);

but the code has (so "maxRows" != "chunkSize"):

+PQsetChunkedRowsMode(PGconn *conn, int chunkSize)

-J.

Re: Synchronizing slots from primary to standby

[shveta malik]

On Thu, Feb 8, 2024 at 4:03 PM Amit Kapila  wrote:
>
> Few comments on 0001
> ===

Thanks Amit. Addressed these in v81.

> 1.
> + * the slots on the standby and synchronize them. This is done on every call
> + * to SQL function pg_sync_replication_slots.
> >
>
> I think the second sentence can be slightly changed to: "This is done
> by a call to SQL function pg_sync_replication_slots." or "One can call
> SQL function pg_sync_replication_slots to invoke this functionality."

Done.

> 2.
> +update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
> {
> ...
> + SpinLockAcquire(>mutex);
> + slot->data.plugin = plugin_name;
> + slot->data.database = remote_dbid;
> + slot->data.two_phase = remote_slot->two_phase;
> + slot->data.failover = remote_slot->failover;
> + slot->data.restart_lsn = remote_slot->restart_lsn;
> + slot->data.confirmed_flush = remote_slot->confirmed_lsn;
> + slot->data.catalog_xmin = remote_slot->catalog_xmin;
> + slot->effective_catalog_xmin = remote_slot->catalog_xmin;
> + SpinLockRelease(>mutex);
> +
> + if (remote_slot->catalog_xmin != slot->data.catalog_xmin)
> + ReplicationSlotsComputeRequiredXmin(false);
> +
> + if (remote_slot->restart_lsn != slot->data.restart_lsn)
> + ReplicationSlotsComputeRequiredLSN();
> ...
> }
>
> How is it possible that after assigning the values from remote_slot
> they can differ from local slot values?

It was a mistake while comment fixing in previous versions. Corrected
it now. Thanks for catching.

> 3.
> + /*
> + * Find the oldest existing WAL segment file.
> + *
> + * Normally, we can determine it by using the last removed segment
> + * number. However, if no WAL segment files have been removed by a
> + * checkpoint since startup, we need to search for the oldest segment
> + * file currently existing in XLOGDIR.
> + */
> + oldest_segno = XLogGetLastRemovedSegno() + 1;
> +
> + if (oldest_segno == 1)
> + oldest_segno = XLogGetOldestSegno(0);
>
> I feel this way isn't there a risk that XLogGetOldestSegno() will get
> us the seg number from some previous timeline which won't make sense
> to compare segno in reserve_wal_for_local_slot. Shouldn't you need to
> fetch the current timeline and send as a parameter to this function as
> that is the timeline on which standby is communicating with primary.

Yes, modified it.

> 4.
> + if (remote_slot->confirmed_lsn > latestFlushPtr)
> + ereport(ERROR,
> + errmsg("skipping slot synchronization as the received slot sync"
>
> I think the internal errors should be reported with elog as you have
> done at other palces in the patch.

Done.

> 5.
> +synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
> {
> ...
> + /*
> + * Copy the invalidation cause from remote only if local slot is not
> + * invalidated locally, we don't want to overwrite existing one.
> + */
> + if (slot->data.invalidated == RS_INVAL_NONE)
> + {
> + SpinLockAcquire(>mutex);
> + slot->data.invalidated = remote_slot->invalidated;
> + SpinLockRelease(>mutex);
> +
> + /* Make sure the invalidated state persists across server restart */
> + ReplicationSlotMarkDirty();
> + ReplicationSlotSave();
> + slot_updated = true;
> + }
> ...
> }
>
> Do we need to copy the 'invalidated' from remote to local if both are
> same? I think this will happen for each slot each time because
> normally slots won't be invalidated ones, so there is needless writes.

It is not needed everytime. Optimized it. Now we copy only if
local_slot's 'invalidated' value is RS_INVAL_NONE while remote-slot's
value != RS_INVAL_NONE.

> 6.
> + * Returns TRUE if any of the slots gets updated in this sync-cycle.
> + */
> +static bool
> +synchronize_slots(WalReceiverConn *wrconn)
> ...
> ...
>
> +void
> +SyncReplicationSlots(WalReceiverConn *wrconn)
> +{
> + PG_TRY();
> + {
> + validate_primary_slot_name(wrconn);
> +
> + (void) synchronize_slots(wrconn);
>
> For the purpose of 0001, synchronize_slots() doesn't seems to use
> return value. So, I suggest to change it accordingly and move the
> return value in the required patch.

Modified it. Also changed return values of all related internal
functions which were returning slot_updated.

> 7.
> + /*
> + * The primary_slot_name is not set yet or WALs not received yet.
> + * Synchronization is not possible if the walreceiver is not started.
> + */
> + latestWalEnd = GetWalRcvLatestWalEnd();
> + SpinLockAcquire(>mutex);
> + if ((WalRcv->slotname[0] == '\0') ||
> + XLogRecPtrIsInvalid(latestWalEnd))
> + {
> + SpinLockRelease(>mutex);
> + return false;
>
> For the purpose of 0001, we should give WARNING here.

I will fix it in the next version. Sorry, I somehow missed it this time.

thanks
Shveta

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Thu, Feb 8, 2024 at 4:08 AM Nathan Bossart 
wrote:

> Mats, I apologize for steamrolling a bit here.  I'll take a step back into
> a reviewer role.
>

No worries. :)


>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>

Re: Psql meta-command conninfo+

[Jim Jones]

Hi Maiquel

On 07.02.24 21:13, Maiquel Grassi wrote:
>
> I believe in v7 patch we have a quite substantial meta-command feature.
>
>
Thanks for implementing this. I find it very handy.

I was just wondering if a "permission denied" error for non-superusers
is the best choice for "\conninfo+":

postgres=> \conninfo+
ERROR:  permission denied to examine "unix_socket_directories"
DETAIL:  Only roles with privileges of the "pg_read_all_settings" role
may examine this parameter.

.. since it is not the case with "\conninfo":

postgres=> \conninfo
You are connected to database "postgres" as user "jim" via socket in
"/tmp" at port "5432".

Perhaps excluding the column from the result set or returning NULL in
the affected columns would be less confusing?

There are also some indentation issues in your patch:

/home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:142:
indent with spaces.
    PGresult   *res;
/home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:143:
indent with spaces.
    PQExpBufferData buf;
/home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:144:
indent with spaces.
    printQueryOpt myopt = pset.popt;
/home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:146:
indent with spaces.
    initPQExpBuffer();
/home/jim/Downloads/v7-0001-psql-meta-command-conninfo-plus.patch:148:
indent with spaces.
    printfPQExpBuffer(,
warning: squelched 34 whitespace errors
warning: 39 lines add whitespace errors.

Looking forward to see the documentation and tests!

-- 
Jim

Re: Psql meta-command conninfo+

[Pavel Luzanov]

Hi,

On 07.02.2024 23:13, Maiquel Grassi wrote:

Regarding the "system_user" function, as it is relatively new, I added 
the necessary handling to avoid conflicts with versions lower than 
version 16.



Yes, that's rights.

A couple of doubts about the implementation details.
But keep in mind that I'm not very good at programming in the C language.
I hope for the help of more experienced developers.


1.
+   if (db == NULL)
+   printf(_("You are currently not connected to a 
database.\n"));

This check is performed for \conninfo, but not for \conninfo+.


2.
Some values (address, socket) are evaluated separately for \conninfo
(via C functions) and for \conninfo+ (via sql functions).
It may be worth evaluating them in one place. But I'm not sure about that.

The final version of the patch will require changes to the documentation and 
tests.

--
Pavel Luzanov
Postgres Professional:https://postgrespro.com

Re: Synchronizing slots from primary to standby

[shveta malik]

On Thu, Feb 8, 2024 at 12:08 PM Peter Smith  wrote:
>
> Here are some review comments for patch v80_2-0001.

Thanks for the feedback Peter. Addressed the comments in v81.
Attached patch001 for early feedback. Rest of the patches need
rebasing and thus will post those later.

It also addresses comments by Amit in [1].

[1]: 
https://www.postgresql.org/message-id/CAA4eK1Ldhh_kf-qG-m5BKY0R1SkdBSx5j%2BEzwpie%2BH9GPWWOYA%40mail.gmail.com

thanks
Shveta


v81-0001-Add-a-slot-synchronization-function.patch
Description: Binary data

Re: glibc qsort() vulnerability

[Mats Kindahl]

On Wed, Feb 7, 2024 at 9:56 PM Nathan Bossart 
wrote:

> On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote:
> > Doesn't hurt to fix the comparison functions, and +1 on using the same
> > pattern everywhere.
>
> I attached a new version of the patch with some small adjustments.  I
> haven't looked through all in-tree qsort() comparators to see if any others
> need to be adjusted, but we should definitely do so as part of this thread.
> Mats, are you able to do this?
>

Sure, I checked them and the only ones remaining are those using int16.
Shall I modify those as well?


> > However, we use our qsort() with user-defined comparison functions, and
> we
> > cannot make any guarantees about what they might do. So we must ensure
> that
> > our qsort() doesn't overflow, no matter what the comparison function
> does.
> >
> > Looking at our ST_SORT(), it seems safe to me.
>
> Cool.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
>

Re: Reducing connection overhead in pg_upgrade compat check phase

[Peter Eisentraut]

On 07.02.24 14:25, Daniel Gustafsson wrote:

On 6 Feb 2024, at 17:47, Daniel Gustafsson  wrote:


On 6 Feb 2024, at 17:32, Nathan Bossart  wrote:

On Fri, Feb 02, 2024 at 12:18:25AM +0530, vignesh C wrote:

With no update to the thread and the patch still not applying I'm
marking this as returned with feedback.  Please feel free to resubmit
to the next CF when there is a new version of the patch.


IMHO this patch is worth trying to get into v17.  I'd be happy to take it
forward if Daniel does not intend to work on it.


I actually had the same thought yesterday and spent some time polishing and
rebasing it.  I'll post an updated rebase shortly with the hopes of getting it
committed this week.


Attached is a v11 rebased over HEAD with some very minor tweaks.  Unless there
are objections I plan to go ahead with this version this week.


A few more quick comments:

I think the .report_text assignments also need a gettext_noop(), like 
the .status assignments.


The type DataTypesUsageChecks is only used in check.c, so doesn't need 
to be in pg_upgrade.h.



Idea for further improvement: Might be nice if the 
DataTypesUsageVersionCheck struct also included the applicable version 
information, so the additional checks in version.c would no longer be 
necessary.

Re: Make documentation builds reproducible

[Peter Eisentraut]

On 23.01.24 02:06, Peter Smith wrote:

This has been working forever, but seems to have broken due to commit
[1] having an undeclared variable.



runtime error: file stylesheet-html-common.xsl line 452 element if
Variable 'autolink.index.see' has not been declared.
make: *** [html-stamp] Error 10


I have committed a fix for this.  I have successfully tested docbook-xsl 
1.77.1 through 1.79.*.

Re: Synchronizing slots from primary to standby

[Amit Kapila]

On Wed, Feb 7, 2024 at 5:32 PM shveta malik  wrote:
>
> Sure, made the suggested function name changes. Since there is no
> other change, I kept the version as v80_2.
>

Few comments on 0001
===
1.
+ * the slots on the standby and synchronize them. This is done on every call
+ * to SQL function pg_sync_replication_slots.
>

I think the second sentence can be slightly changed to: "This is done
by a call to SQL function pg_sync_replication_slots." or "One can call
SQL function pg_sync_replication_slots to invoke this functionality."

2.
+update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
{
...
+ SpinLockAcquire(>mutex);
+ slot->data.plugin = plugin_name;
+ slot->data.database = remote_dbid;
+ slot->data.two_phase = remote_slot->two_phase;
+ slot->data.failover = remote_slot->failover;
+ slot->data.restart_lsn = remote_slot->restart_lsn;
+ slot->data.confirmed_flush = remote_slot->confirmed_lsn;
+ slot->data.catalog_xmin = remote_slot->catalog_xmin;
+ slot->effective_catalog_xmin = remote_slot->catalog_xmin;
+ SpinLockRelease(>mutex);
+
+ if (remote_slot->catalog_xmin != slot->data.catalog_xmin)
+ ReplicationSlotsComputeRequiredXmin(false);
+
+ if (remote_slot->restart_lsn != slot->data.restart_lsn)
+ ReplicationSlotsComputeRequiredLSN();
...
}

How is it possible that after assigning the values from remote_slot
they can differ from local slot values?

3.
+ /*
+ * Find the oldest existing WAL segment file.
+ *
+ * Normally, we can determine it by using the last removed segment
+ * number. However, if no WAL segment files have been removed by a
+ * checkpoint since startup, we need to search for the oldest segment
+ * file currently existing in XLOGDIR.
+ */
+ oldest_segno = XLogGetLastRemovedSegno() + 1;
+
+ if (oldest_segno == 1)
+ oldest_segno = XLogGetOldestSegno(0);

I feel this way isn't there a risk that XLogGetOldestSegno() will get
us the seg number from some previous timeline which won't make sense
to compare segno in reserve_wal_for_local_slot. Shouldn't you need to
fetch the current timeline and send as a parameter to this function as
that is the timeline on which standby is communicating with primary.

4.
+ if (remote_slot->confirmed_lsn > latestFlushPtr)
+ ereport(ERROR,
+ errmsg("skipping slot synchronization as the received slot sync"

I think the internal errors should be reported with elog as you have
done at other palces in the patch.


5.
+synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
{
...
+ /*
+ * Copy the invalidation cause from remote only if local slot is not
+ * invalidated locally, we don't want to overwrite existing one.
+ */
+ if (slot->data.invalidated == RS_INVAL_NONE)
+ {
+ SpinLockAcquire(>mutex);
+ slot->data.invalidated = remote_slot->invalidated;
+ SpinLockRelease(>mutex);
+
+ /* Make sure the invalidated state persists across server restart */
+ ReplicationSlotMarkDirty();
+ ReplicationSlotSave();
+ slot_updated = true;
+ }
...
}

Do we need to copy the 'invalidated' from remote to local if both are
same? I think this will happen for each slot each time because
normally slots won't be invalidated ones, so there is needless writes.

6.
+ * Returns TRUE if any of the slots gets updated in this sync-cycle.
+ */
+static bool
+synchronize_slots(WalReceiverConn *wrconn)
...
...

+void
+SyncReplicationSlots(WalReceiverConn *wrconn)
+{
+ PG_TRY();
+ {
+ validate_primary_slot_name(wrconn);
+
+ (void) synchronize_slots(wrconn);

For the purpose of 0001, synchronize_slots() doesn't seems to use
return value. So, I suggest to change it accordingly and move the
return value in the required patch.

7.
+ /*
+ * The primary_slot_name is not set yet or WALs not received yet.
+ * Synchronization is not possible if the walreceiver is not started.
+ */
+ latestWalEnd = GetWalRcvLatestWalEnd();
+ SpinLockAcquire(>mutex);
+ if ((WalRcv->slotname[0] == '\0') ||
+ XLogRecPtrIsInvalid(latestWalEnd))
+ {
+ SpinLockRelease(>mutex);
+ return false;

For the purpose of 0001, we should give WARNING here.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] LockAcquireExtended improvement

[Jingxian Li]

Hello Robert,

On 2024/2/2 5:05, Robert Haas wrote:
> On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li  wrote:
>> According to what you said, I resubmitted a patch which splits the ProcSleep
>> logic into two parts, the former is responsible for inserting self to
>> WaitQueue,
>> the latter is responsible for deadlock detection and processing, and the
>> former part is directly called by LockAcquireExtended before nowait fails.
>> In this way the nowait case can also benefit from adjusting the insertion
>> order of WaitQueue.
>
> I don't have time for a full review of this patch right now
> unfortunately, but just looking at it quickly:
>
> - It will be helpful if you write a clear commit message. If it gets
> committed, there is a high chance the committer will rewrite your
> message, but in the meantime it will help understanding.
>
> - The comment for InsertSelfIntoWaitQueue needs improvement. It is
> only one line. And it says "Insert self into queue if dontWait is
> false" but then someone will wonder why the function would ever be
> called with dontWait = true.
>
> - Between the comments and the commit message, the division of
> responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
> to be clearly explained. Right now I don't think it is.

Based on your comments above, I improve the commit message and comment for 
InsertSelfIntoWaitQueue in new patch.

--
Jingxian Li

Re: [PATCH] LockAcquireExtended improvement

[Jingxian Li]

Hello Robert,

On 2024/2/2 5:05, Robert Haas wrote:
> On Thu, Feb 1, 2024 at 2:16 AM Jingxian Li  wrote:
>> According to what you said, I resubmitted a patch which splits the ProcSleep
>> logic into two parts, the former is responsible for inserting self to
>> WaitQueue,
>> the latter is responsible for deadlock detection and processing, and the
>> former part is directly called by LockAcquireExtended before nowait fails.
>> In this way the nowait case can also benefit from adjusting the insertion
>> order of WaitQueue.
>
> I don't have time for a full review of this patch right now
> unfortunately, but just looking at it quickly:
>
> - It will be helpful if you write a clear commit message. If it gets
> committed, there is a high chance the committer will rewrite your
> message, but in the meantime it will help understanding.
>
> - The comment for InsertSelfIntoWaitQueue needs improvement. It is
> only one line. And it says "Insert self into queue if dontWait is
> false" but then someone will wonder why the function would ever be
> called with dontWait = true.
>
> - Between the comments and the commit message, the division of
> responsibilities between InsertSelfIntoWaitQueue and ProcSleep needs
> to be clearly explained. Right now I don't think it is.

Based on your comments above, I improve the commit message and comment for 
InsertSelfIntoWaitQueue in new patch.

--
Jingxian Li


v2-0002-LockAcquireExtended-improvement.patch
Description: Binary data

Thoughts about NUM_BUFFER_PARTITIONS

[wenhui qiu]

HI hackers
When I read this text in this document there is a paragraph in it(
https://www.interdb.jp/pg/pgsql08/03.html)
/*
The BufMappingLock is split into partitions to reduce contention in the
buffer table (the default is 128 partitions). Each BufMappingLock partition
guards a portion of the corresponding hash bucket slots.
*/,

Physical servers with terabytes of RAM are now commonplace,I'm looking at
the comments inside the source code.I'm looking at the comments inside the
source code to see if they still match the current hardware capability? The
 comment says that in the future there may be a parameter,Iam a  dba now
and I try to think of implementing this parameter, but I'm not a
professional kernel developer, I still want the community senior developer
to implement this parameter

/*
 * It's a bit odd to declare NUM_BUFFER_PARTITIONS and NUM_LOCK_PARTITIONS
 * here, but we need them to figure out offsets within MainLWLockArray, and
 * having this file include lock.h or bufmgr.h would be backwards.
 */

/* Number of partitions of the shared buffer mapping hashtable */
#define NUM_BUFFER_PARTITIONS  128

/*
 * The number of partitions for locking purposes.  This is set to match
 * NUM_BUFFER_PARTITIONS for now, on the basis that whatever's good enough
for
 * the buffer pool must be good enough for any other purpose.  This could
 * become a runtime parameter in future.
 */
#define DSHASH_NUM_PARTITIONS_LOG2 7
#define DSHASH_NUM_PARTITIONS (1 << DSHASH_NUM_PARTITIONS_LOG2)

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[Ashutosh Bapat]

On Thu, Feb 8, 2024 at 9:57 AM Laurenz Albe  wrote:
>
> On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > -   how to set the replica identity.  If a table without a replica identity 
> > is
> > +   how to set the replica identity.  If a table without a replica identity
> > +   (or with replica identity behavior the same as 
> > NOTHING) is
> > added to a publication that replicates UPDATE
> > or DELETE operations then
> > subsequent UPDATE or DELETE
>
> I had the impression that the root of the confusion was the perceived 
> difference
> between "REPLICA IDENTITY NOTHING" and "no replica identity", and that change
> doesn't improve that.
>
> How about:
>
>   If a table without a replica identity (explicitly set to 
> NOTHING,
>   or set to a primary key or index that doesn't exist) is added ...

Another possibility is just to improve the documentation of various
options as follows.

DEFAULT

If there is a primary key, record the old values of the columns of the
primary key. Otherwise it acts as NOTHING. This is the default for
non-system tables.

USING INDEX index_name

Records the old values of the columns covered by the named index, that
must be unique, not partial, not deferrable, and include only columns
marked NOT NULL. If this index is dropped, the behavior is the same as
NOTHING.

FULL

Records the old values of all columns in the row.

NOTHING

Records no information about the old row. This is equivalent to having
no replica identity. This is the default for system tables.


-- 
Best Wishes,
Ashutosh Bapat

RE: Improve eviction algorithm in ReorderBuffer

[Hayato Kuroda (Fujitsu)]

Dear Sawada-san,

Thanks for making v3 patchset. I have also benchmarked the case [1].
Below results are the average of 5th, there are almost the same result
even when median is used for the comparison. On my env, the regression
cannot be seen.

HEAD (1e285a5)  HEAD + v3 patches   difference
10910.722 ms10714.540 msaround 1.8%

Also, here are mino comments for v3 set.

01.
bh_nodeidx_entry and ReorderBufferMemTrackState is missing in typedefs.list.

02. ReorderBufferTXNSizeCompare
Should we assert {ta, tb} are not NULL?

[1]: 
https://www.postgresql.org/message-id/CAD21AoB-7mPpKnLmBNfzfavG8AiTwEgAdVMuv%3DjzmAp9ex7eyQ%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Re: Race condition in FetchTableStates() breaks synchronization of subscription tables

[vignesh C]

On Tue, 6 Feb 2024 at 18:30, Alexander Lakhin  wrote:
>
> 05.02.2024 13:13, vignesh C wrote:
> > Thanks for the steps for the issue, I was able to reproduce this issue
> > in my environment with the steps provided. The attached patch has a
> > proposed fix where the latch will not be set in case of the apply
> > worker exiting immediately after starting.
>
> It looks like the proposed fix doesn't help when ApplyLauncherWakeup()
> called by a backend executing CREATE SUBSCRIPTION command.
> That is, with the v4-0002 patch applied and pg_usleep(30L); added
> just below
>  if (!worker_in_use)
>  return worker_in_use;
> I still observe the test 027_nosuperuser running for 3+ minutes:
> t/027_nosuperuser.pl .. ok
> All tests successful.
> Files=1, Tests=19, 187 wallclock secs ( 0.01 usr  0.00 sys +  4.82 cusr  4.47 
> csys =  9.30 CPU)
>
> IIUC, it's because a launcher wakeup call, sent by "CREATE SUBSCRIPTION
> regression_sub ...", gets missed when launcher waits for start of another
> worker (logical replication worker for subscription "admin_sub"), launched
> just before that command.

Yes, the wakeup call sent by the "CREATE SUBSCRIPTION" command was
getting missed in this case. The wakeup call can be sent during
subscription creation/modification and when the apply worker exits.
WaitForReplicationWorkerAttach should not reset the latch here as it
will end up delaying the apply worker to get started after 180 seconds
timeout(DEFAULT_NAPTIME_PER_CYCLE). The attached patch does not reset
the latch and lets ApplyLauncherMain to reset the latch and checks if
any new worker or missing worker needs to be started.

Regards,
Vignesh
From f04db050a583c9c01eb77766f830a0cf77b0a6c7 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 5 Feb 2024 14:55:48 +0530
Subject: [PATCH v5 2/2] Apply worker will get started after 180 seconds by the
 launcher in case the apply worker exits immediately after startup.

Apply worker was getting started after 180 seconds tiemout of the
launcher in case the apply worker exits immediately after startup. This
was happening because the launcher process's latch was getting reset
after the apply worker was started, which resulted in the launcher to
wait for the next 180 seconds timeout before starting the apply worker.
Fixed this issue by not resetting the latch, as this latch will be set for
subscription modifications and apply worker exit. We should check if the
new worker needs to be started or not and reset the latch in ApplyLauncherMain.
---
 src/backend/replication/logical/launcher.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 122db0bb13..a754f2c757 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -191,7 +191,6 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
 			   BackgroundWorkerHandle *handle)
 {
 	BgwHandleStatus status;
-	int			rc;
 
 	for (;;)
 	{
@@ -226,16 +225,14 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
 		/*
 		 * We need timeout because we generally don't get notified via latch
 		 * about the worker attach.  But we don't expect to have to wait long.
+		 * Since this latch is also used for subscription creation/modification
+		 * and apply worker process exit signal handling, the latch must not be
+		 * reset here. We should check if the new worker needs to be started or
+		 * not and reset the latch in ApplyLauncherMain.
 		 */
-		rc = WaitLatch(MyLatch,
-	   WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
-	   10L, WAIT_EVENT_BGWORKER_STARTUP);
-
-		if (rc & WL_LATCH_SET)
-		{
-			ResetLatch(MyLatch);
-			CHECK_FOR_INTERRUPTS();
-		}
+		(void) WaitLatch(MyLatch,
+		 WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+		 10L, WAIT_EVENT_BGWORKER_STARTUP);
 	}
 }
 
-- 
2.34.1

From 385c96a71396ea8efa86d6136bbf0bfe5282f1d1 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Thu, 1 Feb 2024 09:46:40 +0530
Subject: [PATCH v5 1/2] Table sync missed for newly added tables because
 subscription relation cache invalidation was not handled in certain
 concurrent scenarios.

Table sync was missed if the invalidation of table sync occurs while
the non ready tables were getting prepared. This occurrs because
the table state was being set to valid at the end of non ready table
list preparation irrespective of any inavlidations occurred while
preparing the list. Fixed it by changing the boolean variable to an
tri-state enum and by setting table state to valid only if no
invalidations have been occurred while the list is being prepared.
---
 src/backend/replication/logical/tablesync.c | 25 +
 src/tools/pgindent/typedefs.list|  1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 

Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability

[jian he]

On Thu, Feb 8, 2024 at 1:27 PM Jeevan Chalke
 wrote:
>
>
>
> On Wed, Feb 7, 2024 at 9:13 PM jian he  wrote:
>>
>> On Wed, Feb 7, 2024 at 7:36 PM Jeevan Chalke
>>  wrote:
>> > Added checkTimezoneIsUsedForCast() check where ever we are casting 
>> > timezoned to non-timezoned types and vice-versa.
>>
>> https://www.postgresql.org/docs/devel/functions-json.html
>> above Table 9.51. jsonpath Filter Expression Elements, the Note
>> section, do we also need to rephrase it?
>
>
> OK. Added a line for the same.
>

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6788ba8..37ae2d1 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18240,7 +18240,11 @@ ERROR:  jsonpath member accessor can only be
applied to an object
   timestamptz, and time to timetz.
   However, all but the first of these conversions depend on the current
setting, and thus can only be performed
-  within timezone-aware jsonpath functions.
+  within timezone-aware jsonpath functions.  Similarly, other
+  date/time-related methods that convert string to the date/time types
+  also do the casting and may involve the current
+  .  To preserve the immutability, those can
+  only be performed within timezone-aware jsonpath functions.
  
 

my proposed minor changes:
-  within timezone-aware jsonpath functions.
+  within timezone-aware jsonpath functions. Similarly, other
+  date/time-related methods that convert string to the date/time types
+  also do the casting and may involve the current
+   setting. Those conversions can
+  only be performed within timezone-aware jsonpath functions.
I don't have a strong opinion, though.

Re: A comment in DropRole() contradicts the actual behavior

[Kyotaro Horiguchi]

At Thu, 8 Feb 2024 16:39:23 +0900, Michael Paquier  wrote 
in 
> On Thu, Feb 08, 2024 at 09:00:01AM +0300, Alexander Lakhin wrote:
> > Hello,
> > 
> > Please look at errors, which produced by the following script, starting
> > from 6566133c5:
> > for i in `seq 100`; do (echo "CREATE USER u; DROP USER u;"); done | psql 
> > >psql-1.log 2>&1 &
> > for i in `seq 100`; do (echo "CREATE USER u; DROP USER u;"); done | psql 
> > >psql-2.log 2>&1 &
> > wait
> > 
> > ERROR:  could not find tuple for role 16387
> > ERROR:  could not find tuple for role 16390
> > ERROR:  could not find tuple for role 16394
> > ...
> > 
> > Maybe these errors are expected, but then I'm confused by the comment in
> > DropRole():
> 
> Well, these errors should never happen, IMHO, so it seems to me that
> the comment is right and that the code lacks locking, contrary to what
> the comment tells.

The function acquires a lock, but it does not perform an existence
check until it first attempts to fetch the tuple, believing in its
presence. However, I doubt performing an additional existence check
right after acquiring the lock is worthwhile.

By the way, I saw the following error with the provided script:

> ERROR:  duplicate key value violates unique constraint 
> "pg_authid_rolname_index"
> DETAIL:  Key (rolname)=(u) already exists.
> STATEMENT:  CREATE USER u;

This seems to be another instance of a similar thinko.

I vaguely think that we should just regard the absence as a concurrent
drop and either adjust or remove the message, then fix the
comment. The situation is slightly different for the duplication
case. We shouldn't ignore it but rather need to adjust the error
message.  As long as these behaviors don't lead to inconsistencies.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: confusing / inefficient "need_transcoding" handling in copy

[Sutou Kouhei]

Hi,

In <20240208.172501.2177371292839763981@clear-code.com>
  "Re: confusing / inefficient "need_transcoding" handling in copy" on Thu, 08 
Feb 2024 17:25:01 +0900 (JST),
  Sutou Kouhei  wrote:

> How about the following to avoid needless transcoding?

Oh, sorry. I missed the Michael's patch:
https://www.postgresql.org/message-id/flat/ZcR9Q9hJ8GedFSCd%40paquier.xyz#e73272b042a22befac7a95f7bcb4fb9a

I withdraw my change.


Thanks,
-- 
kou

Re: confusing / inefficient "need_transcoding" handling in copy

[Heikki Linnakangas]

On 08/02/2024 09:05, Michael Paquier wrote:

On Tue, Feb 06, 2024 at 02:24:45PM -0800, Andres Freund wrote:

I think the code is just very confusing - there actually *is* verification of
the encoding, it just happens at a different, earlier, layer, namely in
copyfromparse.c: CopyConvertBuf() which says:
/*
 * If the file and server encoding are the same, no encoding conversion 
is
 * required.  However, we still need to verify that the input is valid 
for
 * the encoding.
 */

And does indeed verify that.


This has been switched by Heikki in f82de5c46bdf, in 2021, that has
removed pg_database_encoding_max_length() in the COPY FROM case.
(Adding him now in CC, in case he has any comments).


Yeah, I agree the "pg_database_encoding_max_length() > 1" check in COPY 
TO is unnecessary. It's always been like that, but now that the COPY TO 
and COPY FROM cases don't share the code, it's more obvious. Let's 
remove it.


On your patch:


+* Set up encoding conversion info, validating data if server and
+* client encodings are not the same (see also pg_server_to_any).


There's no validation, just conversion. I'd suggest:

"Set up encoding conversion info if the file and server encodings differ 
(see also pg_server_to_any)."


Other than that, +1


That's a performance-only change, but there may be a good argument for
backpatching something, perhaps?


-1 for backpatching, out of principle. This is not a regression, it's 
always been like that. Seems innocent, but why is this different from 
any other performance improvement we make.



BTW, I can see an optimization opportunity even if the encodings differ: 
Currently, CopyAttributeOutText() calls pg_server_to_any(), and then 
grovels through the string to find any characters that need to be 
quoted. You could do it the other way round and handle quoting before 
the conversion. That has two benefits:


1. You don't need the strlen() call, because you just scanned through 
the string so you already know its length.
2. You don't need to worry about 'encoding_embeds_ascii' when you 
operate on the server encoding.


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: confusing / inefficient "need_transcoding" handling in copy

[Sutou Kouhei]

Hi,

In <20240206222445.hzq22pb2nye7r...@awork3.anarazel.de>
  "Re: confusing / inefficient "need_transcoding" handling in copy" on Tue, 6 
Feb 2024 14:24:45 -0800,
  Andres Freund  wrote:

> One unfortunate issue: We don't have any tests verifying that COPY FROM
> catches encoding issues.

How about the attached patch for it?


How about the following to avoid needless transcoding?


diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index bd4710a79b..80ec26cafe 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -612,13 +612,14 @@ BeginCopyTo(ParseState *pstate,
cstate->file_encoding = cstate->opts.file_encoding;
 
/*
-* Set up encoding conversion info.  Even if the file and server 
encodings
-* are the same, we must apply pg_any_to_server() to validate data in
-* multibyte encodings.
+* Set up encoding conversion info. We use pg_server_to_any() for the
+* conversion. pg_server_to_any() does nothing with an encoding that
+* equals to GetDatabaseEncoding() and PG_SQL_ASCII. If
+* cstate->file_encoding equals to GetDatabaseEncoding() and 
PG_SQL_ASCII,
+* we don't need to transcode.
 */
-   cstate->need_transcoding =
-   (cstate->file_encoding != GetDatabaseEncoding() ||
-pg_database_encoding_max_length() > 1);
+   cstate->need_transcoding = !(cstate->file_encoding == 
GetDatabaseEncoding() ||
+
cstate->file_encoding == PG_SQL_ASCII);
/* See Multibyte encoding comment above */
cstate->encoding_embeds_ascii = 
PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding);
 


Numbers on my environment:

master:  861.646ms
patched: 697.547ms

SQL:
COPY (SELECT 
1::int2,2::int2,3::int2,4::int2,5::int2,6::int2,7::int2,8::int2,9::int2,10::int2,11::int2,12::int2,13::int2,14::int2,15::int2,16::int2,17::int2,18::int2,19::int2,20::int2,
 generate_series(1, 100::int4)) TO '/dev/null' \watch c=5


Thanks,
-- 
kou
From 387f11bde4eb928af23f6a66101aa9d63b3dcfdf Mon Sep 17 00:00:00 2001
From: Sutou Kouhei 
Date: Thu, 8 Feb 2024 17:17:25 +0900
Subject: [PATCH v1] Add a test for invalid encoding for COPY FROM

The test data uses UTF-8 "hello" in Japanese but the test specifies
EUC_JP. So it's an invalid data.
---
 src/test/regress/expected/copyencoding.out |  8 
 src/test/regress/parallel_schedule |  2 +-
 src/test/regress/sql/copyencoding.sql  | 10 ++
 3 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 src/test/regress/expected/copyencoding.out
 create mode 100644 src/test/regress/sql/copyencoding.sql

diff --git a/src/test/regress/expected/copyencoding.out b/src/test/regress/expected/copyencoding.out
new file mode 100644
index 00..aa4ecea09e
--- /dev/null
+++ b/src/test/regress/expected/copyencoding.out
@@ -0,0 +1,8 @@
+--
+-- Test cases for COPY WITH (ENCODING)
+--
+CREATE TABLE test (t text);
+COPY test FROM stdin WITH (ENCODING 'EUC_JP');
+ERROR:  invalid byte sequence for encoding "EUC_JP": 0xe3 0x81
+CONTEXT:  COPY test, line 1
+DROP TABLE test;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 1d8a414eea..238cef28c4 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -36,7 +36,7 @@ test: geometry horology tstypes regex type_sanity opr_sanity misc_sanity comment
 # execute two copy tests in parallel, to check that copy itself
 # is concurrent safe.
 # --
-test: copy copyselect copydml insert insert_conflict
+test: copy copyselect copydml copyencoding insert insert_conflict
 
 # --
 # More groups of parallel tests
diff --git a/src/test/regress/sql/copyencoding.sql b/src/test/regress/sql/copyencoding.sql
new file mode 100644
index 00..07c85e988b
--- /dev/null
+++ b/src/test/regress/sql/copyencoding.sql
@@ -0,0 +1,10 @@
+--
+-- Test cases for COPY WITH (ENCODING)
+--
+
+CREATE TABLE test (t text);
+COPY test FROM stdin WITH (ENCODING 'EUC_JP');
+こんにちは
+\.
+
+DROP TABLE test;
-- 
2.43.0