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

[Peter Eisentraut]

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/.  This makes the logic simpler, and it also makes
the behavior consistent with the meson build system.  For example, a 
file like schemapg.h is now mentioned only in


src/include/catalog/{meson.build,Makefile,.gitignore}

where before it was mentioned in (checks ...)

src/backend/catalog/.gitignore
src/backend/catalog/Makefile
src/include/Makefile
src/include/catalog/.gitignore
src/include/catalog/meson.build

Also, the list of catalog files is now kept in parallel in
src/include/catalog/{meson.build,Makefile}, while before the makefiles
had it in src/backend/catalog/Makefile.

I think keeping the two build systems aligned this way will be useful 
for longer-term maintenance.


(There are other generated header files that are linked in a similar way 
and could perhaps be simplified.  But they don't all work the same way. 
Some of the scripts also generate .c files, for example, so they need to 
put some stuff under src/backend/.  So I restricted this patch to 
src/{backend,include}/catalog/, especially because it would be good to 
keep the catalog lists aligned.)From e9f3bfeeff47d96c2557836e7c964adba2d9edb5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 8 Feb 2024 08:31:21 +0100
Subject: [PATCH] Put genbki.pl output into src/include/catalog/ directly

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 changes it so that the output files are written directly to
src/include/catalog/.  This makes the logic simpler, and it also makes
the behavior consistent with the meson build system.  Also, the list
of catalog files is now kept in parallel in
src/include/catalog/{meson.build,Makefile}, while before the makefiles
had it in src/backend/catalog/Makefile.
---
 src/backend/Makefile   |   2 +-
 src/backend/catalog/.gitignore |   8 --
 src/backend/catalog/Makefile   | 140 +-
 src/include/Makefile   |   7 +-
 src/include/catalog/.gitignore |   4 +-
 src/include/catalog/Makefile   | 152 +
 src/include/catalog/meson.build|   3 +-
 src/tools/pginclude/cpluspluscheck |   2 -
 src/tools/pginclude/headerscheck   |   2 -
 9 files changed, 163 insertions(+), 157 deletions(-)
 delete mode 100644 src/backend/catalog/.gitignore

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 7d2ea7d54a6..6b3c432c545 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -138,7 +138,7 @@ utils/activity/wait_event_types.h: 
utils/activity/generate-wait_event_types.pl u
 
 # run this unconditionally to avoid needing to know its dependencies here:
 submake-catalog-headers:
-   $(MAKE) -C catalog generated-header-symlinks
+   $(MAKE) -C ../include/catalog generated-headers
 
 # run this unconditionally to avoid needing to know its dependencies here:
 submake-nodes-headers:
diff --git a/src/backend/catalog/.gitignore b/src/backend/catalog/.gitignore
deleted file mode 100644
index b580f734c71..000
--- a/src/backend/catalog/.gitignore
+++ /dev/null
@@ -1,8 +0,0 @@
-/postgres.bki
-/schemapg.h
-/syscache_ids.h
-/syscache_info.h
-/system_fk_info.h
-/system_constraints.sql
-/pg_*_d.h
-/bki-stamp
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index 196ecafc909..1589a75fd53 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -50,141 +50,8 @@ OBJS = \
 
 include $(top_srcdir)/src/backend/common.mk
 
-# Note: the order of this list determines the order in which the catalog
-# header files are assembled into postgres.bki.  BKI_BOOTSTRAP catalogs
-# must appear first, and pg_statistic before pg_statistic_ext_data, and
-# there are reputedly other, undocumented ordering dependencies.
-CATALOG_HEADERS := \
-   pg_proc.h \
-   pg_type.h \
-   pg_attribute.h \
-   pg_class.h \
-   pg_attrdef.h \
-   pg_constraint.h \
-   pg_inherits.h \
-   pg_index.h \
-   pg_operator.h \
-   pg_opfamily.h \
-   pg_opclass.h \
-   pg_am.h \
-   pg_amop.h \
-   pg_amproc.h \
-   pg_language.h \
-   pg_largeobject_metadata.h \
-   pg_largeobject.h \
-   pg_aggregate.h \
-   pg_statistic.h \
-   pg_statistic_ext.h \
-   pg_statistic_ext_data.h \
-   pg_rewrite.h \
-   pg_trigger.h \
-   pg_event_trigger.h \
-   pg_description.h \
-   pg_cast.h \
-   pg_enum.h \
-   pg_namespace.h \
-   pg_conversion.h \
-   pg_depend.h \
-   pg_database.h \
-   pg_db_role_setting.h \
-   pg_tablespace.h \
-   pg_authid.h \
-   pg_auth_members.h \
-   pg_shdepend.h \
-   pg_shdescription.h \
-   pg_ts_config.h 

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

[Michael Paquier]

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.
--
Michael


signature.asc
Description: PGP signature

Re: table inheritance versus column compression and storage settings

[Ashutosh Bapat]

On Wed, Feb 7, 2024 at 12:47 PM Ashutosh Bapat
 wrote:

> 0001 fixes compression inheritance
> 0002 fixes storage inheritance
>

The first patch does not update compression_1.out which makes CI
unhappy. Here's patchset fixing that.


-- 
Best Wishes,
Ashutosh Bapat
From 629db9289371d3e72dc6b0d43811fb6decd94525 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Thu, 1 Feb 2024 12:58:36 +0530
Subject: [PATCH 2/2] Column storage and inheritance

The storage method specified while creating a child table overrides any
parent storage method, even if parents have conflicting storage methods.
If the child doesn't specify a storage method and all its parents use
the same storage method, the child inherits parents' storage method. We
don't allow a child without storage specification when its parents use
conflicting storage methods.

The storage property remains unchanged in a child inheriting from
parent/s after its creation i.e. using ALTER TABLE ... INHERIT.

NOTEs to reviewer (may be included in the final commit message.)

Before this commit the specified storage method could be stored in
ColumnDef::storage (CREATE TABLE ... LIKE) or ColumnDef::storage_name
(CREATE TABLE ...). This caused the MergeChildAttribute() and
MergeInheritedAttribute() to ignore storage method specified in the
child definition since it looked only at ColumnDef::storage. This commit
removes ColumnDef::storage and instead uses ColumnDef::storage_name to
save any storage method specification. This is similar to how
compression method specification is handled.

Before this commit the error detail would mention the first pair of
conflicting parent storage methods. But with this commit it waits till
the child specification is considered by which time we may have
encountered many such conflicting pairs. Hence the error detail after
this commit does not report conflicting storage methods. Those can be
obtained from parent definitions if necessary. The code to maintain list
of all conflicting methods or that to remember even just the first pair
does not seem worth the effort. This change is inline with what we do
with conflicting default values.

Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c83...@eisentraut.org
---
 doc/src/sgml/ref/create_table.sgml|  5 +-
 src/backend/catalog/pg_type.c | 22 +
 src/backend/commands/tablecmds.c  | 86 +++
 src/backend/nodes/makefuncs.c |  2 +-
 src/backend/parser/gram.y |  7 +-
 src/backend/parser/parse_utilcmd.c|  4 +-
 src/include/catalog/pg_type.h |  2 +
 src/include/nodes/parsenodes.h|  3 +-
 .../regress/expected/create_table_like.out| 12 +--
 src/test/regress/expected/inherit.out | 38 
 src/test/regress/sql/create_table_like.sql|  2 +-
 src/test/regress/sql/inherit.sql  | 20 +
 12 files changed, 131 insertions(+), 72 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 3e5e4b107e..e676a6098c 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -398,7 +398,10 @@ WITH ( MODULUS numeric_literal, REM
  
 
  
-  Column STORAGE settings are also copied from parent tables.
+  Explicitly specified Column STORAGE settings overrides
+  those inherited from parent tables. Otherwise all the parents should have
+  the same Column STORAGE setting which will be
+  inherited by the column in the new table, or an error will be reported.
  
 
  
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index d7167108fb..cfa3220ea5 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -957,3 +957,25 @@ makeMultirangeTypeName(const char *rangeTypeName, Oid typeNamespace)
 
 	return pstrdup(buf);
 }
+
+/*
+ * GetAttributeStorageName
+ *	  returns the name corresponding to a typstorage/attstorage enum value.
+ */
+const char *
+GetAttributeStorageName(char c)
+{
+	switch (c)
+	{
+		case TYPSTORAGE_PLAIN:
+			return "PLAIN";
+		case TYPSTORAGE_EXTERNAL:
+			return "EXTERNAL";
+		case TYPSTORAGE_EXTENDED:
+			return "EXTENDED";
+		case TYPSTORAGE_MAIN:
+			return "MAIN";
+		default:
+			return NULL;
+	}
+}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dc1e97d3f4..ac4075b988 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -351,12 +351,14 @@ typedef struct ForeignTruncateInfo
 	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
 
 /*
- * Bogus compression method name to track conflict in inherited compression
- * property of attributes. It can be any string which does not look like a valid
- * compression method. It is meant to be used by MergeAttributes() and its
- * minions. It is not expected to be stored on disk.
+ * Bogus property 

Re: pg_stat_advisor extension

[Andrei Lepikhov]

On 6/2/2024 22:27, Ilia Evdokimov wrote:


I welcome your insights, feedback, and evaluations regarding the 
necessity of integrating this new extension into PostgreSQL.
Besides other issues that were immediately raised during the discovery 
of the extension, Let me emphasize two issues:
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().
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.


--
regards,
Andrei Lepikhov
Postgres Professional

Re: Testing autovacuum wraparound (including failsafe)

[Peter Eisentraut]

On 08.02.24 05:05, Masahiko Sawada wrote:

On Thu, Feb 8, 2024 at 3:11 AM Peter Eisentraut  wrote:


The way src/test/modules/xid_wraparound/meson.build is written, it
installs the xid_wraparound.so module into production installations.
For test modules, a different installation code needs to be used.  See
neighboring test modules such as
src/test/modules/test_rbtree/meson.build for examples.



Good catch, thanks.

I've attached the patch to fix it. Does it make sense?


Yes, that looks correct to me and produces the expected behavior.

Re: confusing / inefficient "need_transcoding" handling in copy

[Michael Paquier]

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).

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

Oops.

Anyway, I was looking at the copyto.c code because I need to get
something on this thread to be able to do something about the
pluggable APIs in COPY, and echoing with what you mentioned upthread,
what we only need to do is to set need_transcoding only when the
client and the server encodings are not the same?  Am I missing
something?

Runtime gets much better in average, around 1260ms on HEAD vs 1023ms
with the attached for the example of upthread on a single process.
Some profile data from CopyOneRowTo(), if relevant:
* HEAD:
-   82.78%10.96%  postgres  postgres[.] CopyOneRowTo
- 71.82% CopyOneRowTo
   + 30.87% OutputFunctionCall
   - 13.21% CopyAttributeOutText
pg_server_to_any
   - 9.48% appendBinaryStringInfo
4.93% enlargeStringInfo
 3.33% 0xa4e1e234
   + 3.20% CopySendEndOfRow
 2.66% 0xa4e1e214
 1.02% pgstat_progress_update_param
 0.86% memcpy@plt
 0.74% 0xa4e1cba4
 0.72% MemoryContextReset
 0.72% 0xa4e1cba8
 0.59% enlargeStringInfo
 0.55% 0xa4e1cb40
 0.54% 0xa4e1cb74
 0.52% 0xa4e1cb8c
+ 10.96% _start
* patch:
-   80.82%12.25%  postgres  postgres[.] CopyOneRowTo
- 68.57% CopyOneRowTo
   + 36.55% OutputFunctionCall
 11.44% CopyAttributeOutText
   + 8.87% appendBinaryStringInfo
   + 3.79% CopySendEndOfRow
 1.01% pgstat_progress_update_param
 0.79% int2out
 0.66% MemoryContextReset
 0.63% 0xaa624ba8
 0.60% memcpy@plt
 0.60% enlargeStringInfo
 0.53% 0xaa624ba4
+ 12.25% _start

That's a performance-only change, but there may be a good argument for
backpatching something, perhaps?
--
Michael
From 6ddbcd4d6333bc96c21ca95d632d83f1e1459064 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 8 Feb 2024 16:03:39 +0900
Subject: [PATCH] Speedup COPY TO

---
 src/backend/commands/copyto.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index bd4710a79b..c6b45cab53 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -612,13 +612,15 @@ 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, validating data if server and
+	 * client encodings are not the same (see also pg_server_to_any).
 	 */
-	cstate->need_transcoding =
-		(cstate->file_encoding != GetDatabaseEncoding() ||
-		 pg_database_encoding_max_length() > 1);
+	if (cstate->file_encoding == GetDatabaseEncoding() ||
+		cstate->file_encoding == PG_SQL_ASCII)
+		cstate->need_transcoding = false;
+	else
+		cstate->need_transcoding = true;
+
 	/* See Multibyte encoding comment above */
 	cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding);
 
-- 
2.43.0



signature.asc
Description: PGP signature

Re: What about Perl autodie?

[Peter Eisentraut]

On 08.02.24 07:03, Tom Lane wrote:

John Naylor  writes:

On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane  wrote:

No drawbacks. I've been using it heavily for many, many years. Came out in 
5.10.1,
which should be available everywhere at this point (2009 was the year of 
release)



We moved our minimum to 5.14 fairly recently, so we're good on that point.


Yeah, but only recently.  I'm a little worried about the value of this
change relative to the amount of code churn involved, and more to the
point I worry about the risk of future back-patches injecting bad code
into back branches that don't use autodie.

(Back-patching the use of autodie doesn't seem feasible, since before
v16 we supported perl 5.8.something.)


Yeah, good points.  I suppose we could start using it for completely new 
scripts.

Re: Synchronizing slots from primary to standby

[Peter Smith]

Here are some review comments for patch v80_2-0001.

==
Commit message

1.
We may also see the the slots invalidated and dropped on the standby if the
primary changes 'wal_level' to a level lower than logical. Changing the primary
'wal_level' to a level lower than logical is only possible if the logical slots
are removed on the primary server, so it's expected to see the slots
being removed
on the standby too (and re-created if they are re-created on the
primary server).


~

Typo /the the/the/

==
src/sgml/logicaldecoding.sgml

2.
+
+ The logical replication slots on the primary can be synchronized to
+ the hot standby by enabling failover during slot
+ creation (e.g. using the failover parameter of
+ 
+ pg_create_logical_replication_slot, or
+ using the 
+ failover option of the CREATE SUBSCRIPTION
+ command), and then calling 
+ pg_sync_replication_slots
+ on the standby. For the synchronization to work, it is mandatory to
+ have a physical replication slot between the primary and the standby, and
+ hot_standby_feedback
+ must be enabled on the standby. It is also necessary to specify a valid
+ dbname in the
+ primary_conninfo.
+

Shveta previously asked: Regarding link to create-sub, the
'sql-createsubscription-params-with-failover' takes you to the
failover property of Create-Subscription page. Won't that suffice?

PS: Yes, the current links in 80_2 are fine.

~

2a.
In hindsight, maybe it is simpler just to say "option of CREATE
SUBSCRIPTION." instead of "option of the CREATE SUBSCRIPTION command."

~

2b.
Anyway, the "CREATE SUBSCRIPTION" should be rendered as a 

==

3.
+/*
+ * Flag to tell if we are syncing replication slots. Unlike the 'syncing' flag
+ * in SlotSyncCtxStruct, this flag is true only if the current process is
+ * performing slot synchronization. This flag is also used as safe-guard
+ * to clean-up shared 'syncing' flag of SlotSyncCtxStruct if some problem
+ * happens while we are in the process of synchronization.
+ */

3a.
It looks confusing to use the same word "process" to mean 2 different things.

SUGGESTION
This flag is also used as a safeguard to reset the shared 'syncing'
flag of SlotSyncCtxStruct if some problem occurs while synchronizing.

~

3b.
TBH, I didn't think that 2nd sentence comment needed to be here -- it
seemed more appropriate to say this comment inline where it does this
logic in the function SyncReplicationSlots()

~~~

4. local_sync_slot_required

+/*
+ * Helper function to check if local_slot is required to be retained.
+ *
+ * Return false either if local_slot does not exist on the remote_slots list or
+ * is invalidated while the corresponding remote slot in the list is still
+ * valid, otherwise return true.
+ */

/does not exist on the remote_slots list/does not exist in the
remote_slots list/

/while the corresponding remote slot in the list is still valid/while
the corresponding remote slot is still valid/

~~~

5.
+ bool locally_invalidated = false;
+ bool remote_exists = false;
+

IMO it is more natural to declare these in the other order since the
function logic assigns/tests them in the other order.

~~~

6.
+
+ if (!remote_exists || locally_invalidated)
+ return false;
+
+ return true;

IMO it would be both simpler and easier to understand if this was
written as one line:

return remote_exists && !locally_invalidated;

~~~

7.
+ * Note: Change of 'wal_level' on the primary server to a level lower than
+ * logical may also result in slots invalidation and removal on standby. This
+ * is because such 'wal_level' change is only possible if the logical slots
+ * are removed on the primary server, so it's expected to see the slots being
+ * invalidated and removed on the standby too (and re-created if they are
+ * re-created on the primary).

/may also result in slots invalidation/may also result in slot invalidation/

/removal on standby/removal on the standby/

~~~

8.
+ /* Drop the local slot f it is not required to be retained. */
+ if (!local_sync_slot_required(local_slot, remote_slot_list))

I didn't think this comment was needed because IMO the function name
is self-explanatory. Anyway, if you do want to keep it, then there is
a typo to fix:

/f it is/if it is/

~~~

9.
+ * Update the LSNs and persist the local synced slot for further syncs if the
+ * remote restart_lsn and catalog_xmin have caught up with the local ones,
+ * otherwise do nothing.

Something about "persist ... for further syncs" wording seems awkward
to me but I wasn't sure exactly what it should be. When I fed this
comment into ChatGPT it interpreted "further" as "future" which seemed
better.

e.g.
If the remote restart_lsn and catalog_xmin have caught up with the
local ones, then update the LSNs and store the local synced slot for
future synchronization; otherwise, do nothing.

Maybe that is a better way to express this comment?

~~~

10.
+/*
+ * Validates if all necessary GUCs for slot synchroniz

RE: Synchronizing slots from primary to standby

[Zhijie Hou (Fujitsu)]

On Wednesday, February 7, 2024 9:13 AM Masahiko Sawada  
wrote:
> 
> On Tue, Feb 6, 2024 at 8:21 PM Amit Kapila  wrote:
> >
> > On Tue, Feb 6, 2024 at 3:33 PM Amit Kapila 
> wrote:
> > >
> > > On Tue, Feb 6, 2024 at 1:09 PM Masahiko Sawada
>  wrote:
> > > >
> > > > On Tue, Feb 6, 2024 at 3:19 PM Amit Kapila 
> wrote:
> > > > >
> > > > > I think users can refer to LOGs to see if it has changed since
> > > > > the first time it was configured. I tried by existing parameter
> > > > > and see the following in LOG:
> > > > > LOG:  received SIGHUP, reloading configuration files
> > > > > 2024-02-06 11:38:59.069 IST [9240] LOG:  parameter "autovacuum"
> changed to "on"
> > > > >
> > > > > If the user can't confirm then it is better to follow the steps
> > > > > mentioned in the patch. Do you want something else to be written
> > > > > in docs for this? If so, what?
> > > >
> > > > IIUC even if a wrong slot name is specified to standby_slot_names
> > > > or even standby_slot_names is empty, the standby server might not
> > > > be lagging behind the subscribers depending on the timing. But
> > > > when checking it the next time, the standby server might lag
> > > > behind the subscribers. So what I wanted to know is how the user
> > > > can confirm if a failover-enabled subscription is ensured not to
> > > > go in front of failover-candidate standbys (i.e., standbys using
> > > > the slots listed in standby_slot_names).
> > > >
> > >
> > > But isn't the same explained by two steps ((a) Firstly, on the
> > > subscriber node check the last replayed WAL. (b) Next, on the
> > > standby server check that the last-received WAL location is ahead of
> > > the replayed WAL location on the subscriber identified above.) in
> > > the latest *_0004 patch.
> > >
> >
> > Additionally, I would like to add that the users can use the queries
> > mentioned in the doc after the primary has failed and before promoting
> > the standby. If she wants to do that when both primary and standby are
> > available, the value of 'standby_slot_names' on primary should be
> > referred. Isn't those two sufficient that there won't be false
> > positives?
> 
> From a user perspective, I'd like to confirm the following two points :
> 
> 1. replication slots used by subscribers are synchronized to the standby.
> 2. it's guaranteed that logical replication doesn't go ahead of physical
> replication to the standby.
> 
> These checks are necessary at least when building a replication setup 
> (primary,
> standby, and subscriber). Otherwise, it's too late if we find out that no 
> standby
> is failover-ready when the primary fails and we're about to do a failover.
> 
> As for the point 1 above, we can use the step 1 described in the doc.
> 
> As for point 2, the step 2 described in the doc could return true even if
> standby_slot_names isn't working. For example, standby_slot_names is empty,
> the user changed the standby_slot_names but forgot to reload the config file,
> and the walsender doesn't reflect the standby_slot_names update yet for some
> reason etc. It's possible that standby's last-received WAL location just 
> happens
> to be ahead of the replayed WAL location on the subscriber. So even if the
> check query returns true once, it could return false when we check it again, 
> if
> standby_slot_names is not working. On the other hand, IIUC if the point 2 is
> ensured, the check query always returns true. I think it would be good if we
> could provide a reliable way to check point 2 ideally via SQL queries 
> (especially
> for tools).

Based on off-list discussions with Sawada-san and Amit, an alternative approach 
to
improve this would be collecting the names of the standby slots that each
walsender has waited for, which will be visible in the pg_stat_replication
view. By checking this information, users can confirm that the GUC
standby_slot_names is correctly configured and that logical replication is not
lagging behind the standbys that hold these slots.

To achieve this, we can implement the collection of slot information within
each logical walsender with failover slot acquired, when waiting for he standby
to catch up (WalSndWaitForWal). For each valid standby slot that the walsender
has waited for, we will store the slot names in a shared memory area specific
to each walsender. To optimize performance, we can only rebuild the slot names
if the GUC has changed. We can track this by introducing a flag to monitor GUC
modifications.

When a user queries the pg_stat_replication view, we will retrieve the
collected slot names from the shared memory area associated with each
walsender. However, before returning the slot names, we can verify their
validity once again. If any of the collected slots have been dropped or
invalidated during this time, we will exclude them from the result returned to
the user.

Apart from the above design, I feel since user currently have a way to detect 
this
manually as mentioned in the 0004 patch(we can improve the d

Re: Synchronizing slots from primary to standby

[Nisha Moond]

We conducted stress testing for the patch with a setup of one primary
node with 100 tables and five subscribers, each having 20
subscriptions. Then created three physical standbys syncing the
logical replication slots from the primary node.
All 100 slots were successfully synced on all three standbys. We then
ran the load and monitored LSN convergence using the prescribed SQL
checks.
Once the standbys were failover-ready, we were able to successfully
promote one of the standbys and all the subscribers seamlessly
migrated to the new primary node.

We replicated the tests with 200 tables, creating 200 logical
replication slots. With the increased load, all the tests were
completed successfully.

Minor errors (not due to patch) observed during tests -

1) When the load was run, on subscribers, the logical replication
apply workers started failing due to timeout. This is not related to
the patch as it happened due to the small "wal_receiver_timeout"
setting w.r.t. the load. To confirm, we ran the same load without the
patch too, and the same failure happened.
2) There was a buffer overflow exception on the primary node with the
'200 replication slots' case. It was not related to the patch as it
was due to short memory configuration.

All the tests were done on Windows as well as Linux environments.
Thank you Ajin for the stress test and analysis on Linux.

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

[Ashutosh Bapat]

On Mon, Feb 5, 2024 at 9:21 PM Peter Eisentraut  wrote:
>
> Commit 344d62fb9a9 (2022) introduced unlogged sequences and made it so
> that identity/serial sequences automatically get the persistence level
> of their owning table.  But this works only for CREATE TABLE and not for
> ALTER TABLE / ADD COLUMN.  This patch fixes that.  (should be
> backpatched to 15, 16)

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. 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.

0001 - same as your patch
0002 - additional tests

-- 
Best Wishes,
Ashutosh Bapat
From c142bfc5ec8b3c9c5a01f48693118d132145b45b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 5 Feb 2024 11:22:01 +0100
Subject: [PATCH 1/2] Fix propagation of persistence to sequences in ALTER
 TABLE / ADD COLUMN

Fix for 344d62fb9a9: That commit introduced unlogged sequences and
made it so that identity/serial sequences automatically get the
persistence level of their owning table.  But this works only for
CREATE TABLE and not for ALTER TABLE / ADD COLUMN.  This is fixed
here.

TODO: backpatch to 15, 16
---
 src/backend/parser/parse_utilcmd.c | 11 ++-
 src/test/regress/expected/identity.out | 17 +
 src/test/regress/sql/identity.sql  |  6 ++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 56ac4f516e..c7efd8d8ce 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -459,7 +459,16 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
 	seqstmt = makeNode(CreateSeqStmt);
 	seqstmt->for_identity = for_identity;
 	seqstmt->sequence = makeRangeVar(snamespace, sname, -1);
-	seqstmt->sequence->relpersistence = cxt->relation->relpersistence;
+
+	/*
+	 * Copy the persistence of the table.  For CREATE TABLE, we get the
+	 * persistence from cxt->relation, which comes from the CreateStmt in
+	 * progress.  For ALTER TABLE, the parser won't set
+	 * cxt->relation->relpersistence, but we have cxt->rel as the existing
+	 * table, so we copy the persistence from there.
+	 */
+	seqstmt->sequence->relpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
+
 	seqstmt->options = seqoptions;
 
 	/*
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 08f95674ca..84b59dca13 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -365,6 +365,23 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
 
 ALTER TABLE itest3 ALTER COLUMN a TYPE text;  -- error
 ERROR:  identity column type must be smallint, integer, or bigint
+-- check that unlogged propagates to sequence
+CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
+ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest17
+Unlogged table "public.itest17"
+ Column |  Type   | Collation | Nullable |   Default
++-+---+--+--
+ a  | integer |   | not null | generated always as identity
+ b  | text|   |  | 
+
+\d itest17_a_seq
+   Unlogged sequence "public.itest17_a_seq"
+  Type   | Start | Minimum |  Maximum   | Increment | Cycles? | Cache 
+-+---+-++---+-+---
+ integer | 1 |   1 | 2147483647 | 1 | no  | 1
+Sequence for identity column: public.itest17.a
+
 -- kinda silly to change property in the same command, but it should work
 ALTER TABLE itest3
   ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY,
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 9f35214751..7596f0a36f 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -214,6 +214,12 @@ SELECT seqtypid::regtype FROM pg_sequence WHERE seqrelid = 'itest3_a_seq'::regcl
 
 ALTER TABLE itest3 ALTER COLUMN a TYPE text;  -- error
 
+-- check that unlogged propagates to sequence
+CREATE UNLOGGED TABLE itest17 (a int NOT NULL, b text);
+ALTER TABLE itest17 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+\d itest17
+\d itest17_a_seq
+
 -- kinda silly to change property in the same command, but it should work
 ALTER TABLE itest3
   ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY,
-- 
2.25.1

From a29aaa16cda9ee70829012a676a14c556cf236c5 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Thu, 8 Feb 2024 11:26:00 +0530
Subject

Re: What about Perl autodie?

[Tom Lane]

John Naylor  writes:
> On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane  
> wrote:
>> No drawbacks. I've been using it heavily for many, many years. Came out in 
>> 5.10.1,
>> which should be available everywhere at this point (2009 was the year of 
>> release)

> We moved our minimum to 5.14 fairly recently, so we're good on that point.

Yeah, but only recently.  I'm a little worried about the value of this
change relative to the amount of code churn involved, and more to the
point I worry about the risk of future back-patches injecting bad code
into back branches that don't use autodie.

(Back-patching the use of autodie doesn't seem feasible, since before
v16 we supported perl 5.8.something.)

regards, tom lane

Re: meson: catalog/syscache_ids.h isn't installed

[Sutou Kouhei]

Hi,

In <4b60e9bd-426c-4d4b-bbbd-1abd06fa0...@eisentraut.org>
  "Re: meson: catalog/syscache_ids.h isn't installed" on Mon, 5 Feb 2024 
17:53:52 +0100,
  Peter Eisentraut  wrote:

> On 05.02.24 02:29, Sutou Kouhei wrote:
>> catalog/syscache_ids.h is referred by utils/syscache.h but
>> it's not installed with Meson.
> 
> This has been fixed.  (Somebody else reported it in a different thread
> also.)

Thanks!

Commit: 1ae5ace7558ea949d2f94af2fd5eb145d5558659
Thread: 
https://www.postgresql.org/message-id/CAJ7c6TMDGmAiozDjJQ3%3DP3cd-1BidC_GpitcAuU0aqq-r1eSoQ%40mail.gmail.com

-- 
kou

A comment in DropRole() contradicts the actual behavior

[Alexander Lakhin]

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():
    /*
     * Re-find the pg_authid tuple.
     *
     * Since we've taken a lock on the role OID, it shouldn't be possible
     * for the tuple to have been deleted -- or for that matter updated --
     * unless the user is manually modifying the system catalogs.
     */
    tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
    if (!HeapTupleIsValid(tuple))
        elog(ERROR, "could not find tuple for role %u", roleid);

Best regards,
Alexander

Re: What about Perl autodie?

[John Naylor]

On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane  wrote:
>
> No drawbacks. I've been using it heavily for many, many years. Came out in 
> 5.10.1,
> which should be available everywhere at this point (2009 was the year of 
> release)

We moved our minimum to 5.14 fairly recently, so we're good on that point.

Re: Support "Right Semi Join" plan shapes

[wenhui qiu]

Hi Alena Rybakina
 I saw this code snippet also disable mergejoin ,I think it same  effect
+ /*
+ * For now we do not support RIGHT_SEMI join in mergejoin.
+ */
+ if (jointype == JOIN_RIGHT_SEMI)
+ {
+ *mergejoin_allowed = false;
+ return NIL;
+ }
+

Regards

Alena Rybakina  于2024年1月30日周二 14:51写道：

> Hi! Thank you for your work on this subject.
>
> I have reviewed your patch and I think it is better to add an Assert for
> JOIN_RIGHT_SEMI to the ExecMergeJoin and ExecNestLoop functions to
> prevent the use of RIGHT_SEMI for these types of connections (NestedLoop
> and MergeJoin).
> Mostly I'm suggesting this because of the set_join_pathlist_hook
> function, which is in the add_paths_to_joinrel function, which allows
> you to create a custom node. What do you think?
>
> --
> Regards,
> Alena Rybakina
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: POC, WIP: OR-clause support for indexes

[Andrei Lepikhov]

On 3/2/2024 02:06, Alena Rybakina wrote:

On 01.02.2024 08:00, jian he wrote:
I added your code to the patch.

Thanks Alena and Jian for the detailed scrutiny!

A couple of questions:
1. As I see, transformAExprIn uses the same logic as we invented but 
allows composite and domain types. Could you add a comment explaining 
why we forbid row types in general, in contrast to the transformAExprIn 
routine?
2. Could you provide the tests to check issues covered by the recent (in 
v.15) changes?


Patch 0001-* in the attachment incorporates changes induced by Jian's 
notes from [1].
Patch 0002-* contains a transformation of the SAOP clause, which allows 
the optimizer to utilize partial indexes if they cover all values in 
this array. Also, it is an answer to Alexander's note [2] on performance 
degradation. This first version may be a bit raw, but I need your 
opinion: Does it resolve the issue?


Skimming through the thread, I see that, in general, all issues have 
been covered for now. Only Robert's note on a lack of documentation is 
still needs to be resolved.


[1] 
https://www.postgresql.org/message-id/CACJufxGXhJ823cdAdp2Ho7qC-HZ3_-dtdj-myaAi_u9RQLn45g%40mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAPpHfduJtO0s9E%3DSHUTzrCD88BH0eik0UNog1_q3XBF2wLmH6g%40mail.gmail.com


--
regards,
Andrei Lepikhov
Postgres Professional
From 0ac511ab94959e87d1525d5ea8c4855643a6ccbe Mon Sep 17 00:00:00 2001
From: Alena Rybakina 
Date: Fri, 2 Feb 2024 22:01:09 +0300
Subject: [PATCH 1/2] Transform OR clause to ANY expressions.

Replace (expr op C1) OR (expr op C2) ... with expr op ANY(C1, C2, ...) on the
preliminary stage of optimization when we are still working with the tree
expression.
Here C is a constant expression, 'expr' is non-constant expression, 'op' is
an operator which returns boolean result and has a commuter (for the case of
reverse order of constant and non-constant parts of the expression,
like 'CX op expr').
Sometimes it can lead to not optimal plan. But we think it is better to have
array of elements instead of a lot of OR clauses. Here is a room for further
optimizations on decomposing that array into more optimal parts.
Authors: Alena Rybakina , Andrey Lepikhov 

Reviewed-by: Peter Geoghegan , Ranier Vilela 
Reviewed-by: Alexander Korotkov , Robert Haas 

Reviewed-by: jian he 
---
 .../postgres_fdw/expected/postgres_fdw.out|  16 +-
 src/backend/nodes/queryjumblefuncs.c  |  27 ++
 src/backend/parser/parse_expr.c   | 327 +-
 src/backend/utils/misc/guc_tables.c   |  11 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/bin/pg_dump/t/002_pg_dump.pl  |   6 +-
 src/include/nodes/queryjumble.h   |   1 +
 src/include/optimizer/optimizer.h |   1 +
 src/test/regress/expected/create_index.out| 156 -
 src/test/regress/expected/inherit.out |   2 +-
 src/test/regress/expected/join.out|  62 +++-
 src/test/regress/expected/partition_prune.out | 219 ++--
 src/test/regress/expected/rules.out   |   4 +-
 src/test/regress/expected/stats_ext.out   |  12 +-
 src/test/regress/expected/sysviews.out|   3 +-
 src/test/regress/expected/tidscan.out |  23 +-
 src/test/regress/sql/create_index.sql |  35 ++
 src/test/regress/sql/join.sql |  10 +
 src/test/regress/sql/partition_prune.sql  |  22 ++
 src/test/regress/sql/tidscan.sql  |   6 +
 src/tools/pgindent/typedefs.list  |   2 +
 21 files changed, 876 insertions(+), 70 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out 
b/contrib/postgres_fdw/expected/postgres_fdw.out
index b5a38aeb21..a07aefc9e5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1349,7 +1349,7 @@ SELECT t1.c1, t1.c2, t2.c1, t2.c2 FROM ft4 t1 LEFT JOIN 
(SELECT * FROM ft5 WHERE
  Foreign Scan
Output: t1.c1, t1.c2, ft5.c1, ft5.c2
Relations: (public.ft4 t1) LEFT JOIN (public.ft5)
-   Remote SQL: SELECT r1.c1, r1.c2, r4.c1, r4.c2 FROM ("S 1"."T 3" r1 LEFT 
JOIN "S 1"."T 4" r4 ON (((r1.c1 = r4.c1)) AND ((r4.c1 < 10 WHERE (((r4.c1 < 
10) OR (r4.c1 IS NULL))) AND ((r1.c1 < 10))
+   Remote SQL: SELECT r1.c1, r1.c2, r4.c1, r4.c2 FROM ("S 1"."T 3" r1 LEFT 
JOIN "S 1"."T 4" r4 ON (((r1.c1 = r4.c1)) AND ((r4.c1 < 10 WHERE (((r4.c1 
IS NULL) OR (r4.c1 < 10))) AND ((r1.c1 < 10))
 (4 rows)
 
 SELECT t1.c1, t1.c2, t2.c1, t2.c2 FROM ft4 t1 LEFT JOIN (SELECT * FROM ft5 
WHERE c1 < 10) t2 ON (t1.c1 = t2.c1)
@@ -3105,7 +3105,7 @@ select array_agg(distinct (t1.c1)%5) from ft4 t1 full 
join ft5 t2 on (t1.c1 = t2
  Foreign Scan
Output: (array_agg(DISTINCT (t1.c1 % 5))), ((t2.c1 % 3))
Relations: Aggregate on ((public.ft4 t1) FULL JOIN (public.ft5 t2))
-   Remote SQL: SELECT array_agg(DISTINCT (r1.c1 % 5)), (r2.c1 % 3) FROM ("S 
1"."T 3" r1 FULL JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1 WHERE (((r1.c1 

Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability

[Jeevan Chalke]

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.

Thanks

-- 
Jeevan Chalke

*Principal, ManagerProduct Development*



edbpostgres.com


v2-preserve-immutability.patch
Description: Binary data

Re: 2024-02-08 release announcement draft

[Tom Lane]

"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

Re: 2024-02-08 release announcement draft

[Jonathan S. Katz]

On 2/6/24 3:19 AM, jian he wrote:

On Tue, Feb 6, 2024 at 12:43 PM Jonathan S. Katz  wrote:


Hi,

Attached is a draft of the 2024-02-08 release announcement. Please
review for accuracy and notable omissions.

Please provide any feedback no later than 2024-02-08 12:00 UTC (and
preferably sooner).



* 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.



* Ensure initdb always uncomments `postgresql.conf` entries for the
`lc_` family of parameters.

maybe  `initdb`


I applied this change -- thanks!

Jonathan



OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[Laurenz Albe]

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 ...


Yours,
Laurenz Albe

Re: Testing autovacuum wraparound (including failsafe)

[Masahiko Sawada]

On Thu, Feb 8, 2024 at 3:11 AM Peter Eisentraut  wrote:
>
> The way src/test/modules/xid_wraparound/meson.build is written, it
> installs the xid_wraparound.so module into production installations.
> For test modules, a different installation code needs to be used.  See
> neighboring test modules such as
> src/test/modules/test_rbtree/meson.build for examples.
>

Good catch, thanks.

I've attached the patch to fix it. Does it make sense?

Regards,

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


0001-Prevent-installation-of-xid_wraparound-test-during-m.patch
Description: Binary data

Re: Printing backtrace of postgres processes

[Michael Paquier]

On Thu, Feb 08, 2024 at 12:30:00AM +0530, Bharath Rupireddy wrote:
> I've missed adding LoadBacktraceFunctions() in InitAuxiliaryProcess
> for 0002 patch. Please find the attached v29 patch set. Sorry for the
> noise.

I've been torturing the patch with \watch and loops calling the
function while doing a sequential scan of pg_stat_activity, and that
was stable while doing a pgbench and an installcheck-world in
parallel, with some infinite loops and some spinlocks I should not
have taken.

+   if (backtrace_functions_loaded)
+   return;

I was really wondering about this point, particularly regarding the
fact that this would load libgcc for all the backends when they start,
unconditionally.  One thing could be to hide that behind a postmaster
GUC disabled by default, but then we'd come back to using gdb on a
live server, which is no fun on a customer environment because of the
extra dependencies, which may not, or just cannot, be installed.  So
yeah, I think that I'm OK about that.

+ * procsignal.h
+ *   Backtrace-related routines

This one is incorrect.

In HandleLogBacktraceInterrupt(), we don't use backtrace_symbols() and
rely on backtrace_symbols_fd() to avoid doing malloc() in the signal
handler as mentioned in [1] back in 2022.  Perhaps the part about the
fact that we don't use backtrace_symbols() should be mentioned
explicitely in a comment rather than silently implied?  That's
a very important point.

Echoing with upthread, and we've been more lax with superuser checks
and assignment of custom roles in recent years, I agree with the
approach of the patch to make that superuser by default.  Then users
can force their own policy as they want with an EXECUTE ACL on the SQL
function.

As a whole, I'm pretty excited about being able to rely on that
without the need to use gdb to get a live trace.  Does anybody have
objections and/or comments, particularly about the superuser and the
load part at backend startup?  This thread has been going on for so
long that it would be good to let 1 week for folks to react before
doing anything.  See v29 for references.

[1]: 
https://www.postgresql.org/message-id/CALj2ACUNZVB0cQovvKBd53-upsMur8j-5_K=-fg86uaa+wy...@mail.gmail.com
--
Michael


signature.asc
Description: PGP signature

Re: Change GUC hashtable to use simplehash?

[John Naylor]

On Wed, Feb 7, 2024 at 10:41 PM Peter Eisentraut  wrote:
>
> /tmp/cirrus-ci-build/src/include/common/hashfn_unstable.h: In function
> ‘int fasthash_accum_cstring_unaligned(fasthash_state*, const char*)’:
> /tmp/cirrus-ci-build/src/include/common/hashfn_unstable.h:201:20:
> warning: comparison of integer expressions of different signedness:
> ‘int’ and ‘long unsigned int’ [-Wsign-compare]
> 201 |   while (chunk_len < FH_SIZEOF_ACCUM && str[chunk_len] != '\0')
> |^
>
> and a few more like that.
>
> I think it would be better to declare various int variables and
> arguments as size_t instead.  Even if you don't actually need the larger
> range, it would make it more self-documenting.

Thanks for the report! I can reproduce and have pushed that change.

Re: glibc qsort() vulnerability

[Nathan Bossart]

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

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

RE: speed up a logical replica setup

[Hayato Kuroda (Fujitsu)]

Dear Euler,

>
Remember the target server was a standby (read only access). I don't expect an
application trying to modify it; unless it is a buggy application.
>

What if the client modifies the data just after the promotion?
Naively considered, all the changes can be accepted, but are there any issues?

>
Regarding
GUCs, almost all of them is PGC_POSTMASTER (so it cannot be modified unless the
server is restarted). The ones that are not PGC_POSTMASTER, does not affect the
pg_createsubscriber execution [1].
>

IIUC,  primary_conninfo and primary_slot_name is PGC_SIGHUP.

>
I'm just pointing out that this case is a different from pg_upgrade (from which
this idea was taken). I'm not saying that's a bad idea. I'm just arguing that
you might be preventing some access read only access (monitoring) when it is
perfectly fine to connect to the database and execute queries. As I said
before, the current UI allows anyone to setup the standby to accept only local
connections. Of course, it is an extra step but it is possible. However, once
you apply v16-0007, there is no option but use only local connection during the
transformation. Is it an acceptable limitation?
>

My remained concern is written above. If they do not problematic we may not have
to restrict them for now. At that time, changes 

1) overwriting a port number,
2) setting listen_addresses = ''

are not needed, right? IIUC inconsistency of -P may be still problematic.

>
pglogical_create_subscriber does nothing [2][3].
>

Oh, thanks.
Just to confirm - pglogical set shared_preload_libraries to '', should we 
follow or not?

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

Re: glibc qsort() vulnerability

[Nathan Bossart]

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

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

Re: cfbot is failing all tests on FreeBSD/Meson builds

[Tom Lane]

Thomas Munro  writes:
> On Tue, Jan 30, 2024 at 5:06 PM Tom Lane  wrote:
>> Thomas Munro  writes:
>>> Ahh, there is one: https://github.com/cpan-authors/IO-Tty/issues/38

>> Just for the archives' sake: I hit this today on a fresh install
>> of FreeBSD 14.0, which has pulled in p5-IO-Tty-1.18.  Annoying...

> FWIW here's what I did to downgrade:

Thanks for the recipe --- this worked for me, although I noticed
it insisted on installing perl5.34-5.34.3_3 alongside 5.36.
Doesn't seem to be a problem though --- the main perl installation
is still 5.36.

FWIW, I spent some time yesterday staring at IPC/Run.pm and
came to the (unsurprising) conclusion that there's little we can
do to work around the bug.  None of the moving parts are exposed
to callers.

Also, I'm not entirely convinced that the above-cited issue report is
complaining about the same thing that's biting us.  The reported error
messages are completely different.

regards, tom lane

Re: glibc qsort() vulnerability

[Thomas Munro]

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...

Re: glibc qsort() vulnerability

[Nathan Bossart]

On Wed, Feb 07, 2024 at 06:06:37PM -0800, Andres Freund wrote:
> Another branchless variant is (a > b) - (a < b). It seems to get a similar
> improvement as the overflow-checking version.

Well, that's certainly more elegant.  I updated the patch to that approach
for now.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 8668a0dd83ecbdd356c76416b8398138407ef829 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Wed, 7 Feb 2024 14:29:04 -0600
Subject: [PATCH v3 1/2] remove direct calls to pg_qsort()

---
 contrib/pg_prewarm/autoprewarm.c|  4 ++--
 src/backend/access/brin/brin_minmax_multi.c |  2 +-
 src/backend/storage/buffer/bufmgr.c |  4 ++--
 src/backend/utils/cache/syscache.c  | 10 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 06ee21d496..248b9914a3 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -346,8 +346,8 @@ apw_load_buffers(void)
 	FreeFile(file);
 
 	/* Sort the blocks to be loaded. */
-	pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
-			 apw_compare_blockinfo);
+	qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
+		  apw_compare_blockinfo);
 
 	/* Populate shared memory state. */
 	apw_state->block_info_handle = dsm_segment_handle(seg);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 3ffaad3e42..2c29aa3d4e 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 	 * Sort the distances in descending order, so that the longest gaps are at
 	 * the front.
 	 */
-	pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
+	qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
 
 	return distances;
 }
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eb1ec3b86d..07575ef312 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
 
 	/* sort the list of rlocators if necessary */
 	if (use_bsearch)
-		pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
+		qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
@@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
 
 	/* sort the list of SMgrRelations if necessary */
 	if (use_bsearch)
-		pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
+		qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..662f930864 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -146,14 +146,14 @@ InitCatalogCache(void)
 	Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid));
 
 	/* Sort and de-dup OID arrays, so we can use binary search. */
-	pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheRelationOid, SysCacheRelationOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheRelationOidSize =
 		qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid),
 oid_compare);
 
-	pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheSupportingRelOidSize =
 		qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
 sizeof(Oid), oid_compare);
@@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid)
 
 
 /*
- * OID comparator for pg_qsort
+ * OID comparator for qsort
  */
 static int
 oid_compare(const void *a, const void *b)
-- 
2.25.1

>From a74708f14d6e862b836b2d5106491a8db3114433 Mon Sep 17 00:00:00 2001
From: Mats Kindahl 
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: [PATCH v3 2/2] Ensure comparison functions are transitive

There are a few comparison functions to qsort() that are non-transitive
because they can cause an overflow. Fix these functions to instead use
normal comparisons and return -1, 0, or 1 explicitly.
---
 src/backend/access/spgist/spgtextproc.c |  2 +-
 src/backend/utils/cache/relcache.c  |  9 ++---
 src/bin/pg_upgrade/function.c   | 10 ++
 src/bin/psql/crosstabview.c |  5 -
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..e561bde63e 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -325,7 +325,7 @@ cmpNodePtr(const void *a, const void *b)
 	const 

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[Peter Smith]

On Thu, Feb 8, 2024 at 11:12 AM James Coleman  wrote:
>
> On Wed, Feb 7, 2024 at 6:04 PM Peter Smith  wrote:
> >
> > On Thu, Feb 8, 2024 at 9:04 AM James Coleman  wrote:
> > >
> > > On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe  
> > > wrote:
> > > >
> > > > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > > > We recently noticed some behavior that seems reasonable but also
> > > > > surprised our engineers based on the docs.
> > > > >
> > > > > If we have this setup:
> > > > > create table items(i int);
> > > > > insert into items(i) values (1);
> > > > > create publication test_pub for all tables;
> > > > >
> > > > > Then when we:
> > > > > delete from items where i = 1;
> > > > >
> > > > > we get:
> > > > > ERROR:  cannot delete from table "items" because it does not have a
> > > > > replica identity and publishes deletes
> > > > > HINT:  To enable deleting from the table, set REPLICA IDENTITY using
> > > > > ALTER TABLE.
> > > > >
> > > > > Fair enough. But if we do this:
> > > > > alter table items replica identity nothing;
> > > > >
> > > > > because the docs [1] say that NOTHING means "Records no information
> > > > > about the old row." We still get the same error when we try the DELETE
> > > > > again.
> > > >
> > > > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica 
> > > > identity".
> > > > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > > > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> > > >
> > > > See "pg_class": the column "relreplident" is not nullable.
> > >
> > > Right, I think the confusing point for us is that the docs for NOTHING
> > > ("Records no information about the old row") imply you can decide you
> > > don't have to record anything if you don't want to do so, but the
> > > publication feature is effectively overriding that and asserting that
> > > you can't make that choice.
> > >
> >
> > Hi, I can see how the current docs could be interpreted in a way that
> > was not intended.
> >
> > ~~~
> >
> > To emphasise the DEFAULT behaviour that Laurenze described, I felt
> > there could be another sentence about DEFAULT, the same as there is
> > already for the USING INDEX case.
> >
> > BEFORE [1]
> > Records the old values of the columns of the primary key, if any. This
> > is the default for non-system tables.
> >
> > SUGGESTION
> > Records the old values of the columns of the primary key, if any. This
> > is the default for non-system tables. If there is no primary key, the
> > behavior is the same as NOTHING.
> >
> > ~~~
> >
> > If that is done, then would a publication docs tweak like the one
> > below clarify things sufficiently?
> >
> > BEFORE [2]
> > If a table without a replica identity is added to a publication that
> > replicates UPDATE or DELETE operations then subsequent UPDATE or
> > DELETE operations will cause an error on the publisher.
> >
> > SUGGESTION
> > If a table without a replica identity (or with replica identity
> > behavior equivalent to NOTHING) is added to a publication that
> > replicates UPDATE or DELETE operations then subsequent UPDATE or
> > DELETE operations will cause an error on the publisher.
> >
> > ==
> > [1] 
> > https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
> > [2] 
> > https://www.postgresql.org/docs/current/logical-replication-publication.html
> >
> > Kind Regards,
> > Peter Smith.
> > Fujitsu Australia
>
> Thanks for looking at this!
>
> Yes, both of those changes together would make this unambiguous (and,
> I think, easier to mentally parse).
>

OK, here then is a patch to do like that.

==
Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-replica-identity-clarifications.patch
Description: Binary data

Re: glibc qsort() vulnerability

[Thomas Munro]

On Thu, Feb 8, 2024 at 3:06 PM Andres Freund  wrote:
> On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote:
> > On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:
> > > On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:
> > >> The assembly for that looks encouraging, but I still need to actually 
> > >> test
> > >> it...
> > >
> > > Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
> > > that doesn't work, given the 32bit return, so we need something more.
> >
> > For the same compASC() test, I see an ~8.4% improvement with your int64
> > code
>
> Just to be clear, that code unfortuntely isn't correct, the return value is a
> 32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit
> case.

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

Re: "ERROR: latch already owned" on gharial

[Soumyadeep Chakraborty]

Hey hackers,

I wanted to report that we have seen this issue (with the procLatch) a few
times very sporadically on Greenplum 6X (based on 9.4), with relatively newer
versions of GCC.

I realize that 9.4 is out of support, so this email is purely to add on to the
existing thread, in case the info can help fix/reveal something in supported
versions.

Unfortunately, we don't have a core to share as we don't have the benefit of
commit [1] in Greenplum 6X, but we do possess commit [2] which gives us an elog
ERROR as opposed to PANIC.

Instance 1:

Event 1: 2023-11-13 10:01:31.927168 CET..., pY,
..."LOG","0","disconnection: session time: ..."
Event 2: 2023-11-13 10:01:32.049135
CET...,pX,"FATAL","XX000","latch already owned by pid Y (is_set:
0) (pg_latch.c:159)",,,0,,
"pg_latch.c",159,"Stack trace:
10xbde8b8 postgres errstart (elog.c:567)
20xbe0768 postgres elog_finish (discriminator 7)
30xa08924 postgres  (pg_latch.c:158) <-- OwnLatch
40xa7f179 postgres InitProcess (proc.c:523)
50xa94ac3 postgres PostgresMain (postgres.c:4874)
60xa1e2ed postgres  (postmaster.c:2860)
70xa1f295 postgres PostmasterMain (discriminator 5)
...
"LOG","0","server process (PID Y) exited with exit code
1",,,0,,"postmaster.c",3987,

Instance 2 (was reported with (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20)):

Exactly the same as Instance 1 with identical log, ordering of events and stack
trace, except this time (is_set: 1) when the ERROR is logged.

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.

I'm not sure why we didn't introduce a memory barrier in DisownLatch() in [3].
I didn't find anything in the associated hackers thread [4] either. Was it the
performance impact, or was it just because SetLatch and ResetLatch
were more racy
and this is way less likely to happen?

This is out of my wheelhouse, but would one additional barrier in a process'
lifecycle be that bad for performance?

Appendix:

Build details: (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20)

CFLAGS=-Wall -Wmissing-prototypes -Wpointer-arith -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -fno-aggressive-loop-optimizations
-Wno-unused-but-set-variable -Wno-address -Werror=implicit-fallthrough=3
-Wno-format-truncation -Wno-stringop-truncation -m64 -O3
-fargument-noalias-global -fno-omit-frame-pointer -g -std=gnu99
-Werror=uninitialized -Werror=implicit-function-declaration

Regards,
Soumyadeep (VMware)

[1] 
https://github.com/postgres/postgres/commit/12e28aac8e8eb76cab13a4e9b696e3dab17f1c99
[2] 
https://github.com/greenplum-db/gpdb/commit/81fdd6c5219af865e9dc41f4087e0405d6616050
[3] 
https://github.com/postgres/postgres/commit/14e8803f101a54d99600683543b0f893a2e3f529
[4] 
https://www.postgresql.org/message-id/flat/20150112154026.GB2092%40awork2.anarazel.de
From 9702054c37aa80cb60a16b2d80a475e9b27b087a Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty 
Date: Wed, 7 Feb 2024 18:04:43 -0800
Subject: [PATCH v1 1/1] Use memory barrier in DisownLatch

Failing to do so may cause another process undergoing startup to see our
pid associated with the latch during OwnLatch's sanity check.
---
 src/backend/storage/ipc/latch.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index d8a69990b3a..332c5e0b2cf 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -493,6 +493,12 @@ DisownLatch(Latch *latch)
 	Assert(latch->owner_pid == MyProcPid);
 
 	latch->owner_pid = 0;
+
+	/*
+	 * Ensure that another backend reusing this PGPROC during startup sees that
+	 * owner_pid = 0 and doesn't blow up in OwnLatch().
+	 */
+	pg_memory_barrier();
 }
 
 /*
-- 
2.34.1

Re: glibc qsort() vulnerability

[Andres Freund]

Hi,

On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote:
> On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:
> > On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:
> >> The assembly for that looks encouraging, but I still need to actually test
> >> it...
> > 
> > Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
> > that doesn't work, given the 32bit return, so we need something more.
> 
> For the same compASC() test, I see an ~8.4% improvement with your int64
> code

Just to be clear, that code unfortuntely isn't correct, the return value is a
32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit
case.


> and a ~3.4% improvement with this:

I guess that's still something.

Another branchless variant is (a > b) - (a < b). It seems to get a similar
improvement as the overflow-checking version.

Greetings,

Andres Freund

Re: glibc qsort() vulnerability

[Nathan Bossart]

On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:
> On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:
>> The assembly for that looks encouraging, but I still need to actually test
>> it...
> 
> Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
> that doesn't work, given the 32bit return, so we need something more.

For the same compASC() test, I see an ~8.4% improvement with your int64
code and a ~3.4% improvement with this:

int
compASC(const void *a, const void *b)
{
int result;

if (unlikely(pg_sub_s32_overflow(*(const int32 *) a,
 *(const int32 *) b,
 &result)))
{
if (*(const int32 *) a > *(const int32 *) b)
return 1;
if (*(const int32 *) a < *(const int32 *) b)
return -1;
return 0;
}

return result;
}

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

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[Laurenz Albe]

On Sat, 2024-02-03 at 15:27 -0500, Corey Huinker wrote:
> 
> Here's another attempt, applying Laurenz's feedback:

I like this patch much better.

Some comments:

> --- a/doc/src/sgml/ref/delete.sgml
> +++ b/doc/src/sgml/ref/delete.sgml
> @@ -234,6 +234,24 @@ DELETE FROM films
> In some cases the join style is easier to write or faster to
> execute than the sub-select style.
>
> +  
> +   While there is no LIMIT clause for
> +   DELETE, it is possible to get a similar effect
> +   using the method for UPDATE operations described
> +   in greater detail here.
> +
> +WITH delete_batch AS (
> +  SELECT l.ctid
> +  FROM user_logs AS l
> +  WHERE l.status = 'archived'
> +  ORDER BY l.creation_date
> +  LIMIT 1
> +  FOR UPDATE
> +)
> +DELETE FROM user_logs AS ul
> +USING delete_branch AS del
> +WHERE ul.ctid = del.ctid;
> +
>   
>  
>   

- About the style: there is usually an empty line between an ending 
  and the next starting .  It does not matter for correctness, but I
  think it makes the source easier to read.

- I would rather have only "here" as link text rather than "in greater details
  here".  Even better would be something that gives the reader a clue where
  the link will take her, like
  the documentation of 
UPDATE.

- I am not sure if it is necessary to have the  at all.
  I'd say that it is just a trivial variation of the UPDATE example.
  On the other hand, a beginner might find the example useful.
  Not sure.

If I had my way, I'd just keep the first paragraph, something like

  
   While there is no LIMIT clause for
   DELETE, it is possible to get a similar effect
   using a self-join with a common table expression as described in the
   UPDATE examples.
  


> diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
> index 2ab24b0523..49e0dc29de 100644
> --- a/doc/src/sgml/ref/update.sgml
> +++ b/doc/src/sgml/ref/update.sgml
> @@ -434,7 +434,6 @@ UPDATE wines SET stock = stock + 24 WHERE winename = 
> 'Chateau Lafite 2003';
>  COMMIT;
>  
>
> -
>
> Change the kind column of the table
> films in the row on which the cursor

Please don't.


I'm mostly fine with the UPDATE example.

> +   it can make sense to perform the operation in smaller batches. Performing 
> a
> +   VACUUM operation on the table in between batches can 
> help
> +   reduce table bloat. The

I think the "in" before between is unnecessary and had better be removed, but
I'll defer to the native speaker.

Yours,
Laurenz Albe

Re: cfbot is failing all tests on FreeBSD/Meson builds

[Thomas Munro]

On Tue, Jan 30, 2024 at 5:06 PM Tom Lane  wrote:
> Thomas Munro  writes:
> > On Sat, Jan 13, 2024 at 1:51 PM Tom Lane  wrote:
> >> Time for a bug report to IO::Tty's authors, I guess.
>
> > Ahh, there is one: https://github.com/cpan-authors/IO-Tty/issues/38
>
> Just for the archives' sake: I hit this today on a fresh install
> of FreeBSD 14.0, which has pulled in p5-IO-Tty-1.18.  Annoying...

FWIW here's what I did to downgrade:

  # remove the problematic version (also removes p5-IPC-Run)
  pkg remove -y p5-IO-Tty

  # fetch the known good 1.17 package and install it
  curl -O
"https://pkg.freebsd.org/freebsd:14:x86:64/release_0/All/p5-IO-Tty-1.17.pkg";
  pkg install -y p5-IO-Tty-1.17.pkg

  # put back p5-IPC-Run
  pkg install -y p5-IPC-Run

  # temporarily prevent future "pkg upgrade" from upgrading p5-IO-Tty
  pkg lock -y p5-IO-Tty

Re: Rename setup_cancel_handler in pg_dump

[Yugo NAGATA]

On Wed, 7 Feb 2024 22:59:48 +0100
Daniel Gustafsson  wrote:

> > On 1 Feb 2024, at 02:21, Yugo NAGATA  wrote:
> > On Tue, 30 Jan 2024 13:44:28 +0100
> > Daniel Gustafsson  wrote:
> 
> >> -setup_cancel_handler(void)
> >> +pg_dump_setup_cancel_handler(void)
> >> 
> >> We don't have any other functions prefixed with pg_dump_, based on the 
> >> naming
> >> of the surrounding code in the file I wonder if set_cancel_handler is a 
> >> more
> >> appropriate name?
> > 
> > Agreed. Here is a updated patch.
> 
> Sleeping on it I still think this is a good idea, and hearing no objections I
> went ahead with this.  Thanks for the patch!

Thank you!

Regards,
Yugo Nagata

> 
> --
> Daniel Gustafsson
> 


-- 
Yugo NAGATA 

Re: Should we remove -Wdeclaration-after-statement?

[Noah Misch]

On Mon, Jan 29, 2024 at 04:03:44PM +0100, Jelte Fennema-Nio wrote:
> I feel like this is the type of change where there's not much
> discussion to be had. And the only way to resolve it is to use some
> voting to gauge community opinion.
> 
> So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or
> +1 to indicate support against/for the change.

I'm +1 for the change, for these reasons:

- Fewer back-patch merge conflicts.  The decls section of long functions is a
  classic conflict point.
- A mid-func decl demonstrates that its var is unused in the first half of the
  func.
- We write Perl in the mixed decls style, without problems.

For me personally, the "inconsistency" concern is negligible.  We allowed "for
(int i = 0", and that inconsistency has been invisible to me.

Re: glibc qsort() vulnerability

[Andres Freund]

Hi,

On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:
> On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote:
> > Now, in most cases this won't matter, the sorting isn't performance
> > critical. But I don't think it's a good idea to standardize on a generally
> > slower pattern.
> > 
> > Not that that's a good test, but I did quickly benchmark [1] this with
> > intarray. There's about a 10% difference in performance between using the
> > existing compASC() and one using
> > return (int64) *(const int32 *) a - (int64) *(const int32 *) b;
> > 
> > 
> > Perhaps we could have a central helper for this somewhere?
> 
> Maybe said helper could use __builtin_sub_overflow() and fall back to the
> slow "if" version only if absolutely necessary.

I suspect that'll be worse code in the common case, given the cmov generated
by gcc & clang for the typical branch-y formulation. But it's worth testing.


> The assembly for that looks encouraging, but I still need to actually test
> it...

Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
that doesn't work, given the 32bit return, so we need something more.

Greetings,

Andres Freund

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[James Coleman]

On Wed, Feb 7, 2024 at 6:04 PM Peter Smith  wrote:
>
> On Thu, Feb 8, 2024 at 9:04 AM James Coleman  wrote:
> >
> > On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe  
> > wrote:
> > >
> > > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > > We recently noticed some behavior that seems reasonable but also
> > > > surprised our engineers based on the docs.
> > > >
> > > > If we have this setup:
> > > > create table items(i int);
> > > > insert into items(i) values (1);
> > > > create publication test_pub for all tables;
> > > >
> > > > Then when we:
> > > > delete from items where i = 1;
> > > >
> > > > we get:
> > > > ERROR:  cannot delete from table "items" because it does not have a
> > > > replica identity and publishes deletes
> > > > HINT:  To enable deleting from the table, set REPLICA IDENTITY using
> > > > ALTER TABLE.
> > > >
> > > > Fair enough. But if we do this:
> > > > alter table items replica identity nothing;
> > > >
> > > > because the docs [1] say that NOTHING means "Records no information
> > > > about the old row." We still get the same error when we try the DELETE
> > > > again.
> > >
> > > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> > > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> > >
> > > See "pg_class": the column "relreplident" is not nullable.
> >
> > Right, I think the confusing point for us is that the docs for NOTHING
> > ("Records no information about the old row") imply you can decide you
> > don't have to record anything if you don't want to do so, but the
> > publication feature is effectively overriding that and asserting that
> > you can't make that choice.
> >
>
> Hi, I can see how the current docs could be interpreted in a way that
> was not intended.
>
> ~~~
>
> To emphasise the DEFAULT behaviour that Laurenze described, I felt
> there could be another sentence about DEFAULT, the same as there is
> already for the USING INDEX case.
>
> BEFORE [1]
> Records the old values of the columns of the primary key, if any. This
> is the default for non-system tables.
>
> SUGGESTION
> Records the old values of the columns of the primary key, if any. This
> is the default for non-system tables. If there is no primary key, the
> behavior is the same as NOTHING.
>
> ~~~
>
> If that is done, then would a publication docs tweak like the one
> below clarify things sufficiently?
>
> BEFORE [2]
> If a table without a replica identity is added to a publication that
> replicates UPDATE or DELETE operations then subsequent UPDATE or
> DELETE operations will cause an error on the publisher.
>
> SUGGESTION
> If a table without a replica identity (or with replica identity
> behavior equivalent to NOTHING) is added to a publication that
> replicates UPDATE or DELETE operations then subsequent UPDATE or
> DELETE operations will cause an error on the publisher.
>
> ==
> [1] 
> https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
> [2] 
> https://www.postgresql.org/docs/current/logical-replication-publication.html
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

Thanks for looking at this!

Yes, both of those changes together would make this unambiguous (and,
I think, easier to mentally parse).

Thanks,
James Coleman

Re: pg_stat_advisor extension

[jian he]

On Tue, Feb 6, 2024 at 12:06 AM Ilia Evdokimov
 wrote:
>
> Hi hackers,
>
> I'm reaching out again regarding the patch with new extension 
> 'pg_stat_advisor' aimed at enhancing query plan efficiency through the 
> suggestion of creating statistics.
>
> I understand the community is busy, but I would greatly value any feedback or 
> thoughts on this extension.
>

+ /* Define custom GUC variables. */
+ DefineCustomRealVariable("pg_stat_advisor.suggest_statistics_threshold",
+ "Set the threshold for actual/estimated rows",
+ "Zero disables suggestion of creating statistics",
+ &pg_stat_advisor_suggest_statistics_threshold,
+ 0.0,
+ 0.0,
+ INT_MAX,
+ PGC_SUSET,
+ 0,
+ NULL,
+ NULL,
+ NULL);

INT_MAX
should be 1.0?

+ if (!FindExtendedStatisticsOnVars(&rte->relid, colmap))
+ {
+ ereport(NOTICE, (errmsg("pg_stat_advisor suggestion: CREATE
STATISTICS %s %s FROM %s",
+ stat_name, create_stat_stmt, rel_name),
+ errhidestmt(true)));
+ }
now CREATE STATISTICS, the statistics name is optional.
but here you explicitly mention the statistics kind would be great.

+ elog(DEBUG1, "Estimated=%f, actual=%f, error=%f: plan=%s",
+ plan->plan_rows,
+ planstate->instrument->ntuples,
+ planstate->instrument->ntuples / plan->plan_rows,
+ nodeToString(plan));

` error=%f` seems not that right.
Also since the documentation is limited, more comments explaining
SuggestMultiColumnStatisticsForNode would be great.
overall the comments are very little, it should be more (that's my opinion).

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[Peter Smith]

On Thu, Feb 8, 2024 at 9:04 AM James Coleman  wrote:
>
> On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe  wrote:
> >
> > On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > > We recently noticed some behavior that seems reasonable but also
> > > surprised our engineers based on the docs.
> > >
> > > If we have this setup:
> > > create table items(i int);
> > > insert into items(i) values (1);
> > > create publication test_pub for all tables;
> > >
> > > Then when we:
> > > delete from items where i = 1;
> > >
> > > we get:
> > > ERROR:  cannot delete from table "items" because it does not have a
> > > replica identity and publishes deletes
> > > HINT:  To enable deleting from the table, set REPLICA IDENTITY using
> > > ALTER TABLE.
> > >
> > > Fair enough. But if we do this:
> > > alter table items replica identity nothing;
> > >
> > > because the docs [1] say that NOTHING means "Records no information
> > > about the old row." We still get the same error when we try the DELETE
> > > again.
> >
> > Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> > So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> > "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
> >
> > See "pg_class": the column "relreplident" is not nullable.
>
> Right, I think the confusing point for us is that the docs for NOTHING
> ("Records no information about the old row") imply you can decide you
> don't have to record anything if you don't want to do so, but the
> publication feature is effectively overriding that and asserting that
> you can't make that choice.
>

Hi, I can see how the current docs could be interpreted in a way that
was not intended.

~~~

To emphasise the DEFAULT behaviour that Laurenze described, I felt
there could be another sentence about DEFAULT, the same as there is
already for the USING INDEX case.

BEFORE [1]
Records the old values of the columns of the primary key, if any. This
is the default for non-system tables.

SUGGESTION
Records the old values of the columns of the primary key, if any. This
is the default for non-system tables. If there is no primary key, the
behavior is the same as NOTHING.

~~~

If that is done, then would a publication docs tweak like the one
below clarify things sufficiently?

BEFORE [2]
If a table without a replica identity is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.

SUGGESTION
If a table without a replica identity (or with replica identity
behavior equivalent to NOTHING) is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.

==
[1] 
https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
[2] https://www.postgresql.org/docs/current/logical-replication-publication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: glibc qsort() vulnerability

[Nathan Bossart]

On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote:
> Now, in most cases this won't matter, the sorting isn't performance
> critical. But I don't think it's a good idea to standardize on a generally
> slower pattern.
> 
> Not that that's a good test, but I did quickly benchmark [1] this with
> intarray. There's about a 10% difference in performance between using the
> existing compASC() and one using
>   return (int64) *(const int32 *) a - (int64) *(const int32 *) b;
> 
> 
> Perhaps we could have a central helper for this somewhere?

Maybe said helper could use __builtin_sub_overflow() and fall back to the
slow "if" version only if absolutely necessary.  The assembly for that
looks encouraging, but I still need to actually test it...

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

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[James Coleman]

On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe  wrote:
>
> On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> > We recently noticed some behavior that seems reasonable but also
> > surprised our engineers based on the docs.
> >
> > If we have this setup:
> > create table items(i int);
> > insert into items(i) values (1);
> > create publication test_pub for all tables;
> >
> > Then when we:
> > delete from items where i = 1;
> >
> > we get:
> > ERROR:  cannot delete from table "items" because it does not have a
> > replica identity and publishes deletes
> > HINT:  To enable deleting from the table, set REPLICA IDENTITY using
> > ALTER TABLE.
> >
> > Fair enough. But if we do this:
> > alter table items replica identity nothing;
> >
> > because the docs [1] say that NOTHING means "Records no information
> > about the old row." We still get the same error when we try the DELETE
> > again.
>
> Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
> So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
> "REPLICA IDENTITY USING INDEX ..." if the index is dropped.
>
> See "pg_class": the column "relreplident" is not nullable.

Right, I think the confusing point for us is that the docs for NOTHING
("Records no information about the old row") imply you can decide you
don't have to record anything if you don't want to do so, but the
publication feature is effectively overriding that and asserting that
you can't make that choice.

Regards,
James Coleman

Re: Rename setup_cancel_handler in pg_dump

[Daniel Gustafsson]

> On 1 Feb 2024, at 02:21, Yugo NAGATA  wrote:
> On Tue, 30 Jan 2024 13:44:28 +0100
> Daniel Gustafsson  wrote:

>> -setup_cancel_handler(void)
>> +pg_dump_setup_cancel_handler(void)
>> 
>> We don't have any other functions prefixed with pg_dump_, based on the naming
>> of the surrounding code in the file I wonder if set_cancel_handler is a more
>> appropriate name?
> 
> Agreed. Here is a updated patch.

Sleeping on it I still think this is a good idea, and hearing no objections I
went ahead with this.  Thanks for the patch!

--
Daniel Gustafsson

Re: glibc qsort() vulnerability

[Andres Freund]

Hi,

On 2024-02-07 20:46:56 +0200, Heikki Linnakangas wrote:
> > The routines modified do a subtraction of int:s and return that, which
> > can cause an overflow. This method is used for some int16 as well but
> > since standard conversions in C will perform the arithmetics in "int"
> > precision, this cannot overflow, so added a comment there. It might
> > still be a good idea to follow the same pattern for the int16 routines,
> > but since there is no bug there, I did not add them to the patch.
>
> Doesn't hurt to fix the comparison functions, and +1 on using the same
> pattern everywhere.

It actually can hurt - the generated code will often be slower.

E.g.
#include 

int cmp_sub(int16_t a, int16_t b) {
return (int32_t) a - (int32_t) b;
}

int cmp_if(int16_t a, int16_t b) {
if (a < b)
return -1;
if (a > b)
return 1;
return 0;
}

yields

cmp_sub:
movsx   eax, di
movsx   esi, si
sub eax, esi
ret
cmp_if:
xor eax, eax
cmp di, si
mov edx, -1
setgal
cmovl   eax, edx
ret

with gcc -O3.  With other compilers, e.g. msvc, the difference is considerably
bigger, due to msvc for some reason not using cmov.

See https://godbolt.org/z/34qerPaPE for a few more details.


Now, in most cases this won't matter, the sorting isn't performance
critical. But I don't think it's a good idea to standardize on a generally
slower pattern.

Not that that's a good test, but I did quickly benchmark [1] this with
intarray. There's about a 10% difference in performance between using the
existing compASC() and one using
return (int64) *(const int32 *) a - (int64) *(const int32 *) b;


Perhaps we could have a central helper for this somewhere?

Greetings,

Andres Freund


[1]
-- prep
CREATE EXTENSION IF NOT EXISTS intarray;
DROP TABLE IF EXISTS arrays_to_sort;
CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT 
ARRAY(SELECT generate_series(1, 100)) a), generate_series(1, 10);
-- bench
SELECT (sort(arr))[1] FROM arrays_to_sort;

Re: index prefetching

[Melanie Plageman]

On Wed, Jan 24, 2024 at 3:20 PM Melanie Plageman
 wrote:
>
> On Wed, Jan 24, 2024 at 4:19 AM Tomas Vondra
>  wrote:
> >
> > On 1/24/24 01:51, Melanie Plageman wrote:
> > >> But I'm not sure what to do about optimizations that are more specific
> > >> to the access path. Consider for example the index-only scans. We don't
> > >> want to prefetch all the pages, we need to inspect the VM and prefetch
> > >> just the not-all-visible ones. And then pass the info to the index scan,
> > >> so that it does not need to check the VM again. It's not clear to me how
> > >> to do this with this approach.
> > >
> > > Yea, this is an issue I'll need to think about. To really spell out
> > > the problem: the callback dequeues a TID from the tid_queue and looks
> > > up its block in the VM. It's all visible. So, it shouldn't return that
> > > block to the streaming read API to fetch from the heap because it
> > > doesn't need to be read. But, where does the callback put the TID so
> > > that the caller can get it? I'm going to think more about this.
> > >
> >
> > Yes, that's the problem for index-only scans. I'd generalize it so that
> > it's about the callback being able to (a) decide if it needs to read the
> > heap page, and (b) store some custom info for the TID.
>
> Actually, I think this is no big deal. See attached. I just don't
> enqueue tids whose blocks are all visible. I had to switch the order
> from fetch heap then fill queue to fill queue then fetch heap.
>
> While doing this I noticed some wrong results in the regression tests
> (like in the alter table test), so I suspect I have some kind of
> control flow issue. Perhaps I should fix the resource leak so I can
> actually see the failing tests :)

Attached is a patch which implements a real queue and fixes some of
the issues with the previous version. It doesn't pass tests yet and
has issues. Some are bugs in my implementation I need to fix. Some are
issues we would need to solve in the streaming read API. Some are
issues with index prefetching generally.

Note that these two patches have to be applied before 21d9c3ee4e
because Thomas hasn't released a rebased version of the streaming read
API patches yet.

Issues
---
- kill prior tuple

This optimization doesn't work with index prefetching with the current
design. Kill prior tuple relies on alternating between fetching a
single index tuple and visiting the heap. After visiting the heap we
can potentially kill the immediately preceding index tuple. Once we
fetch multiple index tuples, enqueue their TIDs, and later visit the
heap, the next index page we visit may not contain all of the index
tuples deemed killable by our visit to the heap.

In our case, we could try and fix this by prefetching only heap blocks
referred to by index tuples on the same index page. Or we could try
and keep a pool of index pages pinned and go back and kill index
tuples on those pages.

Having disabled kill_prior_tuple is why the mvcc test fails. Perhaps
there is an easier way to fix this, as I don't think the mvcc test
failed on Tomas' version.

- switching scan directions

If the index scan switches directions on a given invocation of
IndexNext(), heap blocks may have already been prefetched and read for
blocks containing tuples beyond the point at which we want to switch
directions.

We could fix this by having some kind of streaming read "reset"
callback to drop all of the buffers which have been prefetched which
are now no longer needed. We'd have to go backwards from the last TID
which was yielded to the caller and figure out which buffers in the
pgsr buffer ranges are associated with all of the TIDs which were
prefetched after that TID. The TIDs are in the per_buffer_data
associated with each buffer in pgsr. The issue would be searching
through those efficiently.

The other issue is that the streaming read API does not currently
support backwards scans. So, if we switch to a backwards scan from a
forwards scan, we would need to fallback to the non streaming read
method. We could do this by just setting the TID queue size to 1
(which is what I have currently implemented). Or we could add
backwards scan support to the streaming read API.

- mark and restore

Similar to the issue with switching the scan direction, mark and
restore requires us to reset the TID queue and streaming read queue.
For now, I've hacked in something to the PlannerInfo and Plan to set
the TID queue size to 1 for plans containing a merge join (yikes).

- multiple executions

For reasons I don't entirely understand yet, multiple executions (not
rescan -- see ExecutorRun(...execute_once)) do not work. As in Tomas'
patch, I have disabled prefetching (and made the TID queue size 1)
when execute_once is false.

- Index Only Scans need to return IndexTuples

Because index only scans return either the IndexTuple pointed to by
IndexScanDesc->xs_itup or the HeapTuple pointed to by
IndexScanDesc->xs_hitup -- both of which are populated by the index
AM, we have to s

Re: Statistics Import and Export

[Tomas Vondra]

Hi,

I took a quick look at the v4 patches. I haven't done much testing yet,
so only some basic review.

0001

- The SGML docs for pg_import_rel_stats may need some changes. It starts
with description of what gets overwritten (non-)transactionally (which
gets repeated twice), but that seems more like an implementation detail.
But it does not really say which pg_class fields get updated. Then it
speculates about the possible use case (pg_upgrade). I think it'd be
better to focus on the overall goal of updating statistics, explain what
gets updated/how, and only then maybe mention the pg_upgrade use case.

Also, it says "statistics are replaced" but it's quite clear if that
applies only to matching statistics or if all stats are deleted first
and then the new stuff is inserted. (FWIW remove_pg_statistics clearly
deletes all pre-existing stats).


- import_pg_statistics: I somewhat dislike that we're passing arguments
as datum[] array - it's hard to say what the elements are expected to
be, etc. Maybe we should expand this, to make it clear. How do we even
know the array is large enough?

- I don't quite understand why we need examine_rel_attribute. It sets a
lot of fields in the VacAttrStats struct, but then we only use attrtypid
and attrtypmod from it - so why bother and not simply load just these
two fields? Or maybe I miss something.

- examine_rel_attribute can return NULL, but get_attrinfo does not check
for NULL and just dereferences the pointer. Surely that can lead to
segfaults?

- validate_no_duplicates and the other validate functions would deserve
a better docs, explaining what exactly is checked (it took me a while to
realize we check just for duplicates), what the parameters do etc.

- Do we want to make the validate_ functions part of the public API? I
realize we want to use them from multiple places (regular and extended
stats), but maybe it'd be better to have an "internal" header file, just
like we have extended_stats_internal?

- I'm not sure we do "\set debug f" elsewhere. It took me a while to
realize why the query outputs are empty ...


0002

- I'd rename create_stat_ext_entry to statext_create_entry.

- Do we even want to include OIDs from the source server? Why not to
just have object names and resolve those? Seems safer - if the target
server has the OID allocated to a different object, that could lead to
confusing / hard to detect issues.

- What happens if we import statistics which includes data for extended
statistics object which does not exist on the target machine?

- pg_import_ext_stats seems to not use require_match_oids - bug?


0003

- no SGML docs for the new tools?

- The help() seems to be wrong / copied from "clusterdb" or something
like that, right?


On 2/2/24 09:37, Corey Huinker wrote:
> (hit send before attaching patches, reposting message as well)
> 
> Attached is v4 of the statistics export/import patch.
> 
> This version has been refactored to match the design feedback received
> previously.
> 
> The system views are gone. These were mostly there to serve as a baseline
> for what an export query would look like. That role is temporarily
> reassigned to pg_export_stats.c, but hopefully they will be integrated into
> pg_dump in the next version. The regression test also contains the version
> of each query suitable for the current server version.
> 

OK

> The export format is far closer to the raw format of pg_statistic and
> pg_statistic_ext_data, respectively. This format involves exporting oid
> values for types, collations, operators, and attributes - values which are
> specific to the server they were created on. To make sense of those values,
> a subset of the columns of pg_type, pg_attribute, pg_collation, and
> pg_operator are exported as well, which allows pg_import_rel_stats() and
> pg_import_ext_stats() to reconstitute the data structure as it existed on
> the old server, and adapt it to the modern structure and local schema
> objects.

I have no opinion on the proposed format - still JSON, but closer to the
original data. Works for me, but I wonder what Tom thinks about it,
considering he suggested making it closer to the raw data.

> 
> pg_import_rel_stats matches up local columns with the exported stats by
> column name, not attnum. This allows for stats to be imported when columns
> have been dropped, added, or reordered.
> 

Makes sense. What will happen if we try to import data for extended
statistics (or index) that does not exist on the target server?

> pg_import_ext_stats can also handle column reordering, though it currently
> would get confused by changes in expressions that maintain the same result
> data type. I'm not yet brave enough to handle importing nodetrees, nor do I
> think it's wise to try. I think we'd be better off validating that the
> destination extended stats object is identical in structure, and to fail
> the import of that one object if it isn't perfect.
> 

Yeah, column reordering is something we probably need to handle

Re: glibc qsort() vulnerability

[Nathan Bossart]

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?

> 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
>From f5850579e92f201218c3025327b91d820eabd18e Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Wed, 7 Feb 2024 14:29:04 -0600
Subject: [PATCH v2 1/2] remove direct calls to pg_qsort()

---
 contrib/pg_prewarm/autoprewarm.c|  4 ++--
 src/backend/access/brin/brin_minmax_multi.c |  2 +-
 src/backend/storage/buffer/bufmgr.c |  4 ++--
 src/backend/utils/cache/syscache.c  | 10 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 06ee21d496..248b9914a3 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -346,8 +346,8 @@ apw_load_buffers(void)
 	FreeFile(file);
 
 	/* Sort the blocks to be loaded. */
-	pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
-			 apw_compare_blockinfo);
+	qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
+		  apw_compare_blockinfo);
 
 	/* Populate shared memory state. */
 	apw_state->block_info_handle = dsm_segment_handle(seg);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 3ffaad3e42..2c29aa3d4e 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 	 * Sort the distances in descending order, so that the longest gaps are at
 	 * the front.
 	 */
-	pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
+	qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
 
 	return distances;
 }
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eb1ec3b86d..07575ef312 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
 
 	/* sort the list of rlocators if necessary */
 	if (use_bsearch)
-		pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
+		qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
@@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
 
 	/* sort the list of SMgrRelations if necessary */
 	if (use_bsearch)
-		pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
+		qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..662f930864 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -146,14 +146,14 @@ InitCatalogCache(void)
 	Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid));
 
 	/* Sort and de-dup OID arrays, so we can use binary search. */
-	pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheRelationOid, SysCacheRelationOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheRelationOidSize =
 		qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid),
 oid_compare);
 
-	pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheSupportingRelOidSize =
 		qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
 sizeof(Oid), oid_compare);
@@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid)
 
 
 /*
- * OID comparator for pg_qsort
+ * OID comparator for qsort
  */
 static int
 oid_compare(const void *a, const void *b)
-- 
2.25.1

>From f1a046c3c67266187f5861b27c2fad1daa25101c Mon Sep 17 00:00:00 2001
From: Mats Kindahl 
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: [PATCH v2 2/2] Ensure comparison functions are transitive

There are a few comparison functions to qsort() that are non-transitive
because they can cause an overflow. Fix these functions to instead use
normal comparisons and return -1, 0, or 1 explicitly.
---
 src/backend/access/spgist/spgtextproc.c |  6 +-
 src/backend/utils/cache/relcache.c  | 12 
 src/bin/p

Re: speed up a logical replica setup

[Euler Taveira]

On Wed, Feb 7, 2024, at 2:31 AM, Hayato Kuroda (Fujitsu) wrote:
> Ah, actually I did not have such a point of view. Assuming that changed port 
> number
> can avoid connection establishments, there are four options:
> a) Does not overwrite port and listen_addresses. This allows us to monitor by
>external agents, but someone can modify GUCs and data during operations.
> b) Overwrite port but do not do listen_addresses. Not sure it is useful... 
> c) Overwrite listen_addresses but do not do port. This allows us to monitor by
>local agents, and we can partially protect the database. But there is 
> still a 
>room.
> d) Overwrite both port and listen_addresses. This can protect databases 
> perfectly
> but no one can monitor.

Remember the target server was a standby (read only access). I don't expect an
application trying to modify it; unless it is a buggy application. Regarding
GUCs, almost all of them is PGC_POSTMASTER (so it cannot be modified unless the
server is restarted). The ones that are not PGC_POSTMASTER, does not affect the
pg_createsubscriber execution [1].

postgres=# select name, setting, context from pg_settings where name in 
('max_replication_slots', 'max_logical_replication_workers', 
'max_worker_processes', 'max_sync_workers_per_subscription', 
'max_parallel_apply_workers_per_subscription');
name | setting |  context   
-+-+
max_logical_replication_workers | 4   | postmaster
max_parallel_apply_workers_per_subscription | 2   | sighup
max_replication_slots   | 10  | postmaster
max_sync_workers_per_subscription   | 2   | sighup
max_worker_processes| 8   | postmaster
(5 rows)

I'm just pointing out that this case is a different from pg_upgrade (from which
this idea was taken). I'm not saying that's a bad idea. I'm just arguing that
you might be preventing some access read only access (monitoring) when it is
perfectly fine to connect to the database and execute queries. As I said
before, the current UI allows anyone to setup the standby to accept only local
connections. Of course, it is an extra step but it is possible. However, once
you apply v16-0007, there is no option but use only local connection during the
transformation. Is it an acceptable limitation?

Under reflection, I don't expect a big window

1802 /*
1803  * Start subscriber and wait until accepting connections.
1804  */
1805 pg_log_info("starting the subscriber");
1806 if (!dry_run)
1807 start_standby_server(pg_bin_dir, opt.subscriber_dir, 
server_start_log);
1808 
1809 /*
1810  * Waiting the subscriber to be promoted.
1811  */
1812 wait_for_end_recovery(dbinfo[0].subconninfo, pg_bin_dir, &opt);
.
.
.
1845 /*
1846  * Stop the subscriber.
1847  */
1848 pg_log_info("stopping the subscriber");
1849 if (!dry_run)
1850 stop_standby_server(pg_bin_dir, opt.subscriber_dir);

... mainly because the majority of the time will be wasted in
wait_for_end_recovery() if the server takes some time to reach consistent state
(and during this phase it cannot accept connections anyway). Aren't we worrying
too much about it?

> Hmm, which one should be chosen? I prefer c) or d).
> Do you know how pglogical_create_subscriber does?

pglogical_create_subscriber does nothing [2][3].


[1] https://www.postgresql.org/docs/current/logical-replication-config.html
[2] 
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_create_subscriber.c#L488
[3] 
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_create_subscriber.c#L529


--
Euler Taveira
EDB   https://www.enterprisedb.com/

gcc build warnings at -O3

[Andres Freund]

Hi,

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).


> Some of them match up with warnings we're seeing on buildfarm member
> serinus, which I seem to recall that Andres had tracked to a known gcc bug.

Some, but I don't think all.


> In file included from ../../../../src/include/executor/instrument.h:16,
>  from pgstat_io.c:19:
> pgstat_io.c: In function 'pgstat_count_io_op_time':
> pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 
> 'instr_time[2][4][8]' [-Warray-bounds=]
>   149 | 
> INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
>   |^~~

Huh, I don't see that with any version of gcc I tried.


> In file included from ../../../../src/include/access/htup_details.h:22,
>  from pl_exec.c:21:
> In function 'assign_simple_var',
> inlined from 'exec_set_found' at pl_exec.c:8349:2:
> ../../../../src/include/varatt.h:230:36: warning: array subscript 0 is 
> outside array bounds of 'char[0]' [-Warray-bounds=]
>   230 | (((varattrib_1b_e *) (PTR))->va_tag)
>   |^
> ../../../../src/include/varatt.h:94:12: note: in definition of macro 
> 'VARTAG_IS_EXPANDED'
>94 | (((tag) & ~1) == VARTAG_EXPANDED_RO)
>   |^~~
> ../../../../src/include/varatt.h:284:57: note: in expansion of macro 
> 'VARTAG_1B_E'
>   284 | #define VARTAG_EXTERNAL(PTR)
> VARTAG_1B_E(PTR)
>   | ^~~
> ../../../../src/include/varatt.h:301:57: note: in expansion of macro 
> 'VARTAG_EXTERNAL'
>   301 | (VARATT_IS_EXTERNAL(PTR) && 
> !VARTAG_IS_EXPANDED(VARTAG_EXTERNAL(PTR)))
>   | 
> ^~~
> pl_exec.c:8537:17: note: in expansion of macro 
> 'VARATT_IS_EXTERNAL_NON_EXPANDED'
>  8537 | 
> VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
>   | ^~~
> In function 'exec_set_found':
> cc1: note: source object is likely at address zero

This I see.  If I hint to the compiler that var->datatype->typlen != 1 when
called from exec_set_found(), the warning vanishes. E.g. with

if (var->datatype->typlen == -1)
__builtin_unreachable();

I see one more warning:

[1390/2375 42  58%] Compiling C object 
src/backend/postgres_lib.a.p/utils_adt_jsonb_util.c.o
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c: 
In function 'compareJsonbContainers':
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:296:34:
 warning: 'va.type' may be used uninitialized [-Wmaybe-uninitialized]
  296 | res = (va.type > vb.type) ? 1 : -1;
  |~~^
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:204:33:
 note: 'va' declared here
  204 | JsonbValue  va,
  | ^~


I can't really blame the compiler here.  There's a fairly lengthy comment
explaining that va.type/vb.type are set, and it took me a while to understand:

/*
 * It's safe to assume that the types differed, and 
that the va
 * and vb values passed were set.
 *
 * If the two values were of the same container type, 
then there'd
 * have been a chance to observe the variation in the 
number of
 * elements/pairs (when processing WJB_BEGIN_OBJECT, 
say). They're
 * either two heterogeneously-typed containers, or a 
container and
 * some scalar type.
 *
 * We don't have to consider the WJB_END_ARRAY and 
WJB_END_OBJECT
 * cases here, because we would have seen the 
corresponding
 * WJB_BEGIN_ARRAY and WJB_BEGIN_OBJECT tokens first, 
and
 * concluded that they don't match.
 */

It's not surprising that the compiler can't understand that you can't get
ra = WJB_DON

Re: Question about behavior of deletes with REPLICA IDENTITY NOTHING

[Laurenz Albe]

On Wed, 2024-02-07 at 15:12 -0500, James Coleman wrote:
> We recently noticed some behavior that seems reasonable but also
> surprised our engineers based on the docs.
> 
> If we have this setup:
> create table items(i int);
> insert into items(i) values (1);
> create publication test_pub for all tables;
> 
> Then when we:
> delete from items where i = 1;
> 
> we get:
> ERROR:  cannot delete from table "items" because it does not have a
> replica identity and publishes deletes
> HINT:  To enable deleting from the table, set REPLICA IDENTITY using
> ALTER TABLE.
> 
> Fair enough. But if we do this:
> alter table items replica identity nothing;
> 
> because the docs [1] say that NOTHING means "Records no information
> about the old row." We still get the same error when we try the DELETE
> again.

Well, "REPLICA IDENTITY NOTHING" is the same as "has no replica identity".
So is "REPLICA IDENTITY DEFAULT" if there is no primary key, or
"REPLICA IDENTITY USING INDEX ..." if the index is dropped.

See "pg_class": the column "relreplident" is not nullable.

Yours,
Laurenz Albe

RE: Psql meta-command conninfo+

[Maiquel Grassi]

> Hi, Maiquel!
>
> On 07.02.2024 17:47, Maiquel Grassi wrote:

> "Also, it seems that the verbose parameter in the listConnectionInformation 
> is unnecessary."
> Could you point out exactly the line or code snippet you are referring to?

> +bool
> +listConnectionInformation(const char *pattern, bool verbose)
>
> I mean that parameter verbose is not used in the function body and 
> listConnectionInformation called only in verbose mode.


--//--

There really was no need for the bool verbose. Therefore, it was removed.
Regarding the "system_user" function, as it is relatively new, I added
the necessary handling to avoid conflicts with versions lower than version 16.

I believe in v7 patch we have a quite substantial meta-command feature.

I validated the "System User" column for three versions. This way, we have a 
sample:

[postgres@localhost bin]$ ./psql -h 192.168.0.5 -p 5432 -U postgres -d postgres

psql (17devel, server 14.3)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" on host 
"192.168.0.5" at port "5432".
postgres=# \conninfo+
  
Current Connection Information
 Database | Authenticated User | Current User | Session User | Session PID | 
Server Version | Server Address | Server Port | Client Address | Client Port | 
Socket Directory |Host
--++--+--+-+++-++-+--+-
 postgres | postgres   | postgres | postgres |9008 | 
14.3   | 192.168.0.5| 5432| 192.168.0.5|   63631 |  
| 192.168.0.5
(1 row)

postgres=# \q
[postgres@localhost bin]$ ./psql -h 192.168.0.5 -p 5433 -U postgres -d postgres
psql (17devel, server 16.1)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" on host 
"192.168.0.5" at port "5433".
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 |
3348 | 16.1   | 192.168.0.5| 5433| 192.168.0.5| 
  63633 |  | 192.168.0.5
(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 |
   26147 | 17devel| ::1| 5432| ::1| 
  47466 | /tmp | localhost
(1 row)

Regards,
Maiquel O. Grassi.


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

Question about behavior of deletes with REPLICA IDENTITY NOTHING

[James Coleman]

Hello,

We recently noticed some behavior that seems reasonable but also
surprised our engineers based on the docs.

If we have this setup:
create table items(i int);
insert into items(i) values (1);
create publication test_pub for all tables;

Then when we:
delete from items where i = 1;

we get:
ERROR:  cannot delete from table "items" because it does not have a
replica identity and publishes deletes
HINT:  To enable deleting from the table, set REPLICA IDENTITY using
ALTER TABLE.

Fair enough. But if we do this:
alter table items replica identity nothing;

because the docs [1] say that NOTHING means "Records no information
about the old row." We still get the same error when we try the DELETE
again.

The publication docs [2] say "A published table must have a replica
identity configured in order to be able to replicate UPDATE and DELETE
operations, so that appropriate rows to update or delete can be
identified on the subscriber side."

We interpreted the intersection of these two docs to imply that if you
explicitly configured NOTHING that the publication would simply not
log anything about the original row. Part of the confusion I think was
fed by reading "must have a replica identity set" as "have selected
one of the options via ALTER TABLE REPLICA IDENTITY" -- i.e., as
meaning that a setting has been configured rather than being about a
subset of those possible configuration values/a specific key existing
on the table.

I'm wondering if this might be a surprise to anyone else, and if so,
is there a minor docs tweak that might avoid the confusion?

Thanks,
James Coleman

1: 
https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
2: https://www.postgresql.org/docs/current/logical-replication-publication.html

Re: common signal handler protection

[Nathan Bossart]

On Wed, Feb 07, 2024 at 10:40:50AM -0800, Andres Freund wrote:
> On 2024-02-07 11:15:54 -0600, Nathan Bossart wrote:
>> Perhaps we should add a file global bool that is only set during
>> wrapper_handler().  Then we could Assert() or elog(ERROR, ...) if
>> pqsignal() is called with it set.
> 
> In older branches that might have been harder (due to forking from a signal
> handler and non-fatal errors thrown from signal handlers), but these days I
> think that should work.
> 
> FWIW, I don't think elog(ERROR) would be appropriate, that'd be jumping out of
> a signal handler :)

*facepalm*  Yes.

> If it were just for the purpose of avoiding the issue you brought up it might
> not quite be worth it - but there are a lot of things we want to forbid in a
> signal handler. Memory allocations, acquiring locks, throwing non-panic
> errors, etc. That's one of the main reasons I like a common wrapper signal
> handler.
> 
> Which reminded me of https://postgr.es/m/87msstvper.fsf%40163.com - the set of
> things we want to forbid are similar. I'm not sure there's really room to
> harmonize things, but I thought I'd raise it.
> 
> Perhaps we should make the state a bitmap and have a single
>   AssertNotInState(HOLDING_SPINLOCK | IN_SIGNALHANDLER)

Seems worth a try.  I'll go ahead and proceed with these patches and leave
this improvement for another thread.

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

Re: Popcount optimization using AVX512

[Alvaro Herrera]

I happened to notice by chance that John Naylor had posted an extension
to measure performance of popcount here:
https://postgr.es/m/CAFBsxsE7otwnfA36Ly44zZO+b7AEWHRFANxR1h1kxveEV=g...@mail.gmail.com

This might be useful as a base for a new one to verify the results of
the proposed patch in machines with relevant instruction support.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"We're here to devour each other alive"(Hobbes)

Re: the s_lock_stuck on perform_spin_delay

[Andres Freund]

Hi,

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


On 2024-01-25 15:24:17 +0800, Andy Fan wrote:
> From 4c8fd0ab71299e57fbeb18dec70051bd1d035c7c Mon Sep 17 00:00:00 2001
> From: "yizhi.fzh" 
> Date: Thu, 25 Jan 2024 15:19:49 +0800
> Subject: [PATCH v9 1/1] Detect misuse of spin lock automatically
>
> Spin lock are intended for very short-term locks, but it is possible
> to be misused in many cases. e.g. Acquiring another LWLocks or regular
> locks, memory allocation, errstart when holding a spin lock. this patch
> would detect such misuse automatically in a USE_ASSERT_CHECKING build.

> CHECK_FOR_INTERRUPTS should be avoided as well when holding a spin lock.
> Depends on what signals are left to handle, PG may raise error/fatal
> which would cause the code jump to some other places which is hardly to
> release the spin lock anyway.
> ---
>  src/backend/storage/buffer/bufmgr.c | 24 +++
>  src/backend/storage/lmgr/lock.c |  6 +++
>  src/backend/storage/lmgr/lwlock.c   | 21 +++---
>  src/backend/storage/lmgr/s_lock.c   | 63 -
>  src/backend/tcop/postgres.c |  6 +++
>  src/backend/utils/error/elog.c  | 10 +
>  src/backend/utils/mmgr/mcxt.c   | 16 
>  src/include/miscadmin.h | 21 +-
>  src/include/storage/buf_internals.h |  1 +
>  src/include/storage/s_lock.h| 56 ++---
>  src/tools/pgindent/typedefs.list|  2 +-
>  11 files changed, 176 insertions(+), 50 deletions(-)
>
> diff --git a/src/backend/storage/buffer/bufmgr.c 
> b/src/backend/storage/buffer/bufmgr.c
> index 7d601bef6d..739a94209b 100644
> --- a/src/backend/storage/buffer/bufmgr.c
> +++ b/src/backend/storage/buffer/bufmgr.c
> @@ -5402,12 +5402,11 @@ rlocator_comparator(const void *p1, const void *p2)
>  uint32
>  LockBufHdr(BufferDesc *desc)
>  {
> - SpinDelayStatus delayStatus;
>   uint32  old_buf_state;
>
>   Assert(!BufferIsLocal(BufferDescriptorGetBuffer(desc)));
>
> - init_local_spin_delay(&delayStatus);
> + init_local_spin_delay();
>
>   while (true)
>   {

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.



> @@ -5432,20 +5431,29 @@ LockBufHdr(BufferDesc *desc)
>  static uint32
>  WaitBufHdrUnlocked(BufferDesc *buf)
>  {
> - SpinDelayStatus delayStatus;
>   uint32  buf_state;
>
> - init_local_spin_delay(&delayStatus);
> + /*
> +  * 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.



> +#ifdef USE_ASSERT_CHECKING
> +void
> +VerifyNoSpinLocksHeld(bool check_in_panic)
> +{
> + if (!check_in_panic && spinStatus.in_panic)
> + return;

Why do we need this?



> 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.


>  /*
> - * Support for spin delay which is useful in various places where
> - * spinlock-like procedures take place.
> + * Support for spin delay and spin misuse detection purpose.
> + *
> + * spin delay which is useful in various places where spinlock-like
> + * procedures take place.
> + *
> + * spin misuse is based on global spinStatus to know if a spin lock
> + * is held when a heavy operation is taking.
>   */
>  typedef struct
>  {
> @@ -846,22 +854,40 @@ typedef struct
>   const char *file;
>   int line;
>   const char *func;
> -} SpinDelayStatus;
> + boolin_panic; /* works for spin lock misuse purpose. */
> +} SpinLockStatus;
>
> +extern PGDLLIMPORT SpinLockStatus spinStatus;
> +
> +#ifdef USE_ASSERT_CHECKING
> +extern void VerifyNoSpinLocksHeld(bool check_in_panic);
> +extern void ResetSpinLockStatus(void);
> +#else
> +#define VerifyNoSpinLocksHeld(check_in_panic) ((void) true)
> +#define ResetSpinLockStatus() ((void) true)
> +#endif
> +
> +/*
> + * start the spin delay logic and record the places where the spin lock
> + * is held which is a

Re: Printing backtrace of postgres processes

[Bharath Rupireddy]

On Wed, Feb 7, 2024 at 9:00 PM Bharath Rupireddy
 wrote:
>
> On Wed, Feb 7, 2024 at 2:57 PM Michael Paquier  wrote:
> >
> > On Wed, Feb 07, 2024 at 02:04:39PM +0530, Bharath Rupireddy wrote:
> > > Well, that 'ubuntu' is the default username there, I've changed it now
> > > and kept the output short.
> >
> > I would keep it just at two or three lines, with a "For example, with
> > lines like":
>
> Done.
>
> > > I've simplified the tests, now we don't need two separate output files
> > > for tests. Please see the attached v27 patch.
> >
> > +  proname => 'pg_log_backtrace', provolatile => 'v', prorettype => 'bool',
> >
> > Hmm.  Would it be better to be in line with memory contexts logging
> > and use pg_log_backend_backtrace()?
>
> +1.
>
> > One thing I was wondering is that
> > there may be a good point in splitting the backtrace support into two
> > functions (backends and auxiliary processes) that could be split with
> > two execution ACLs across different roles.
>
> -1 for that unless we have any requests. I mean, backtrace is a common
> thing for all postgres processes, why different controls are needed?
> I'd go with what pg_log_backend_memory_contexts does - it supports
> both backends and auxiliary processes.
>
> > +   PROCSIG_LOG_BACKTRACE,  /* ask backend to log the current backtrace 
> > */
> >
> > Incorrect order.
>
> PROCSIG_XXX aren't alphabetically ordered, no?
>
> > +-- Backtrace is not logged for auxiliary processes
> >
> > Not sure there's a point in keeping that in the tests for now.
> >
> > +* XXX: It might be worth implementing it for auxiliary processes.
> >
> > Same, I would remove that.
>
> Done.
>
> > +static volatile sig_atomic_t backtrace_functions_loaded = false;
> >
> > Hmm, so you need that because of the fact that it would be called in a
> > signal as backtrace(3) says:
> > "If you need certain calls to these two functions to not allocate
> > memory (in signal handlers, for example), you need to make sure libgcc
> > is loaded beforehand".
> >
> > True that it is not interesting to only log something when having a
> > CFI, this needs to be dynamic to get a precise state of things.
>
> Right.
>
> I've also fixed some test failures. Please see the attached v28 patch
> set. 0002 extends pg_log_backend_backtrace to auxiliary processes,
> just like pg_log_backend_memory_contexts (not focused on PID
> de-duplication code yet).

I've missed adding LoadBacktraceFunctions() in InitAuxiliaryProcess
for 0002 patch. Please find the attached v29 patch set. Sorry for the
noise.

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


v29-0001-Add-function-to-log-backtrace-of-a-backend.patch
Description: Binary data


v29-0002-Extend-backtrace-logging-function-for-auxiliary-.patch
Description: Binary data

Re: Remove Start* macros from postmaster.c to ease understanding of code

[Nathan Bossart]

On Tue, Feb 06, 2024 at 02:37:25PM -0600, Nathan Bossart wrote:
> Unless there are objections, I'll plan on committing this in the next day
> or two.

Committed.

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

Re: glibc qsort() vulnerability

[Heikki Linnakangas]

On 07/02/2024 11:09, Mats Kindahl wrote:
On Tue, Feb 6, 2024 at 9:56 PM Tom Lane > wrote:


  Nathan Bossart mailto:nathandboss...@gmail.com>> writes:
 > Even if the glibc issue doesn't apply to Postgres, I'm tempted to
suggest
 > that we make it project policy that comparison functions must be
 > transitive.  There might be no real issues today, but if we write all
 > comparison functions the way Mats is suggesting, it should be
easier to
 > reason about overflow risks.

A comparison routine that is not is probably broken, agreed.
I didn't look through the details of the patch --- I was more
curious whether we had a version of the qsort bug, because
if we do, we should fix that too.

The patch basically removes the risk of overflow in three routines and 
just returns -1, 0, or 1, and adds a comment in one.


The routines modified do a subtraction of int:s and return that, which 
can cause an overflow. This method is used for some int16 as well but 
since standard conversions in C will perform the arithmetics in "int" 
precision, this cannot overflow, so added a comment there. It might 
still be a good idea to follow the same pattern for the int16 routines, 
but since there is no bug there, I did not add them to the patch.


Doesn't hurt to fix the comparison functions, and +1 on using the same 
pattern everywhere.


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.

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

Re: common signal handler protection

[Andres Freund]

Hi,

On 2024-02-07 11:15:54 -0600, Nathan Bossart wrote:
> On Wed, Feb 07, 2024 at 11:06:50AM -0600, Nathan Bossart wrote:
> > I'd like to get this committed (to HEAD only) in the next few weeks.  TBH
> > I'm not wild about the weird caveats (e.g., race conditions when pqsignal()
> > is called within a signal handler), but I also think it is unlikely that
> > they cause any issues in practice.  Please do let me know if you have any
> > concerns about this.

I don't.


> Perhaps we should add a file global bool that is only set during
> wrapper_handler().  Then we could Assert() or elog(ERROR, ...) if
> pqsignal() is called with it set.

In older branches that might have been harder (due to forking from a signal
handler and non-fatal errors thrown from signal handlers), but these days I
think that should work.

FWIW, I don't think elog(ERROR) would be appropriate, that'd be jumping out of
a signal handler :)


If it were just for the purpose of avoiding the issue you brought up it might
not quite be worth it - but there are a lot of things we want to forbid in a
signal handler. Memory allocations, acquiring locks, throwing non-panic
errors, etc. That's one of the main reasons I like a common wrapper signal
handler.


Which reminded me of https://postgr.es/m/87msstvper.fsf%40163.com - the set of
things we want to forbid are similar. I'm not sure there's really room to
harmonize things, but I thought I'd raise it.

Perhaps we should make the state a bitmap and have a single
  AssertNotInState(HOLDING_SPINLOCK | IN_SIGNALHANDLER)

Greetings,

Andres Freund

Re: scram_iterations is undocumented GUC_REPORT

[Alvaro Herrera]

On 2024-Feb-07, Daniel Gustafsson wrote:

> On 30 Jan 2024, at 15:44, Alvaro Herrera  wrote:
> 
> > I propose to turn the list into a
> > 
> > which looks _much_ nicer to read, as in the attached screenshot of
> > the PDF.
> 
> +1, this reads a lot better.

Thanks, applied and backpatched to 16.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Refactoring backend fork+exec code

[Andres Freund]

Hi,

On 2024-01-30 02:08:36 +0200, Heikki Linnakangas wrote:
> I spent some more time on the 'lastBackend' optimization in sinvaladt.c. I
> realized that it became very useless with these patches, because aux
> processes are allocated pgprocno's after all the slots for regular backends.
> There are always aux processes running, so lastBackend would always have a
> value close to the max anyway. I replaced that with a dense 'pgprocnos'
> array that keeps track of the exact slots that are in use. I'm not 100% sure
> this is worth the effort; does any real world workload send shared
> invalidations so frequently that this matters? In any case, this should
> avoid the regression if such a workload exists.
>
> New patch set attached. I think these are ready to be committed, but would
> appreciate a final review.


> 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've been thinking that we likely should pad PGPROC to some more convenient
boundary, but...


Is this really related to the rest of the series?


> From 4e0121e064804b73ef8a5dc10be27b85968ea1af Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas 
> Date: Mon, 29 Jan 2024 23:50:34 +0200
> Subject: [PATCH v8 2/5] Redefine backend ID to be an index into the proc
>  array.
>
> Previously, backend ID was an index into the ProcState array, in the
> shared cache invalidation manager (sinvaladt.c). The entry in the
> ProcState array was reserved at backend startup by scanning the array
> for a free entry, and that was also when the backend got its backend
> ID. Things becomes slightly simpler if we redefine backend ID to be
> the index into the PGPROC array, and directly use it also as an index
> to the ProcState array. This uses a little more memory, as we reserve
> a few extra slots in the ProcState array for aux processes that don't
> need them, but the simplicity is worth it.

> Aux processes now also have a backend ID. This simplifies the
> reservation of BackendStatusArray and ProcSignal slots.
>
> 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?


> One potential downside of this patch is that the ProcState array might
> get less densely packed, as we we don't try so hard to assign
> low-numbered backend ID anymore. If it's less densely packed,
> lastBackend will stay at a higher value, and SIInsertDataEntries() and
> SICleanupQueue() need to scan over more unused entries. I think that's
> fine. They are performance critical enough to matter, and there was no
> guarantee on dense packing before either: If you launched a lot of
> backends concurrently, and kept the last one open, lastBackend would
> also stay at a high value.

It's perhaps worth noting here that there's a future patch that also addresses
this to some degree?


> @@ -457,7 +442,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, 
> TransactionId xid, const char *gid,
>   Assert(LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE));
>
>   Assert(gxact != NULL);
> - proc = &ProcGlobal->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?


> 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 + MyAuxProcTy

Re: Testing autovacuum wraparound (including failsafe)

[Peter Eisentraut]

The way src/test/modules/xid_wraparound/meson.build is written, it 
installs the xid_wraparound.so module into production installations. 
For test modules, a different installation code needs to be used.  See 
neighboring test modules such as 
src/test/modules/test_rbtree/meson.build for examples.

Re: Where can I find the doxyfile?

[John Morris]

>> Maybe this is something that can be tweaked on the doxygen side?

I’ll look into that idea. Doxygen is a separate project with its own developers 
to persuade. I could imagine a special “C” or “C++” mode. I wanted to keep 
things simple, and the filter mechanism is how they extend doxygen to support 
other languages.

One alternative is to add the filter to doxygen’s webpage. They currently list 
a number of filters, none of which address this particular issue. The filter 
would become a build dependency like doxygen itself.

In the meantime, I’d like to address Alvaro’s concern about generating 
artifacts. It’s a bit tedious, but a visual survey of the output may add 
confidence or show areas needing improvement.

Re: common signal handler protection

[Nathan Bossart]

Sorry for the noise.

On Wed, Feb 07, 2024 at 11:06:50AM -0600, Nathan Bossart wrote:
> I'd like to get this committed (to HEAD only) in the next few weeks.  TBH
> I'm not wild about the weird caveats (e.g., race conditions when pqsignal()
> is called within a signal handler), but I also think it is unlikely that
> they cause any issues in practice.  Please do let me know if you have any
> concerns about this.

Perhaps we should add a file global bool that is only set during
wrapper_handler().  Then we could Assert() or elog(ERROR, ...) if
pqsignal() is called with it set.

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

Re: Possibility to disable `ALTER SYSTEM`

[Euler Taveira]

On Wed, Feb 7, 2024, at 10:49 AM, Jelte Fennema-Nio wrote:
> On Wed, 7 Feb 2024 at 11:35, Gabriele Bartolini
>  wrote:
> > This is mostly the approach I have taken in the patch, except allowing to 
> > change the value in the configuration file.
> 
> (I had missed the patch in the long thread). I think it would be nice
> to have this be PGC_SIGHUP, and set GUC_DISALLOW_IN_AUTO_FILE. That
> way this behaviour can be changed without shutting down postgres (but
> not with ALTER SYSTEM, because that seems confusing).

Based on Gabriele's use case (Kubernetes) and possible others like a cloud
vendor, I think it should be more restrictive not permissive. I mean,
PGC_POSTMASTER and *only* allow this GUC to be from command-line. (I don't
inspect the code but maybe setting GUC_DISALLOW_IN_FILE is not sufficient to
accomplish this goal.) The main advantages of the GUC system are (a) the
setting is dynamically assigned during startup and (b) you can get the current
setting via SQL.

Another idea is to set it per cluster during initdb like data checksums. You
don't rely on the GUC system but store this information into pg_control. I
think for the referred use cases, you will never have to change it but you can
have a mechanism to change it.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: common signal handler protection

[Nathan Bossart]

On Tue, Feb 06, 2024 at 06:48:53PM -0800, Andres Freund wrote:
> On 2024-02-06 20:39:41 -0600, Nathan Bossart wrote:
>> I finally spent some time trying to measure this overhead.  Specifically, I
>> sent many, many SIGUSR2 signals to postmaster, which just uses
>> dummy_handler(), i.e., does nothing.  I was just barely able to get
>> wrapper_handler() to show up in the first page of 'perf top' in this
>> extreme case, which leads me to think that the overhead might not be a
>> problem.
> 
> That's what I'd expect. Signal delivery is fairly heavyweight, getpid() is one
> of the cheapest system calls (IIRC only beat by close() of an invalid FD on
> recent-ish linux).  If it were to become an issue, we'd much better spend our
> time reducing the millions of signals/sec that'd have to involve.

Indeed.

I'd like to get this committed (to HEAD only) in the next few weeks.  TBH
I'm not wild about the weird caveats (e.g., race conditions when pqsignal()
is called within a signal handler), but I also think it is unlikely that
they cause any issues in practice.  Please do let me know if you have any
concerns about this.

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

Constant Splitting/Refactoring

[David Christensen]

As part of the reserved page space/page features[1] work, there is a need
to convert some of the existing constants into variables, since the size of
those will no longer be fixed at compile time.  At FOSDEM, there was some
interest expressed about seeing what a cleanup patch like that would look
like.

Enclosed is a series of cleanup patches for HEAD.  This splits each of the
4 constants that care about page size into Cluster-specific and -Limit
variants, the first intended to become a variable in time, and the second
being the maximum value such a variable may take (largely used for static
allocations).

Since these patches define these symbols to have the same values
they previously had, there are no changes in functionality.  These were
largely mechanical changes, and as such we should perhaps consider making
the same changes to back-branches to make it so context lines and the like
would be the same, simplifying maintainer's efforts when applying code in
back branches that touch similar areas.

The script I have to make these changes is simple, and could be run against
the back branches with only the comments surrounding Calc() pieces needing
to be adjusted once.

These were based on HEAD as a few minutes ago, 902900b308f, and pass all
tests.
Thanks,

David

[1] https://commitfest.postgresql.org/47/3986/


v1-0002-Split-MaxHeapTupleSize-into-runtime-and-max-value.patch
Description: Binary data


v1-0001-Split-MaxHeapTuplesPerPage-into-runtime-and-max-v.patch
Description: Binary data


v1-0004-Split-MaxTIDsPerBTreePage-into-runtime-and-max-va.patch
Description: Binary data


v1-0003-Split-MaxIndexTuplesPerPage-into-runtime-and-max-.patch
Description: Binary data

Re: What about Perl autodie?

[Greg Sabino Mullane]

On Wed, Feb 7, 2024 at 9:05 AM Peter Eisentraut 
wrote:

> I came across the Perl autodie pragma
> (https://perldoc.perl.org/autodie).  This seems pretty useful; is this
> something we can use?  Any drawbacks?  Any minimum Perl version?


Big +1

No drawbacks. I've been using it heavily for many, many years. Came out in
5.10.1,
which should be available everywhere at this point (2009 was the year of
release)

Cheers,
Greg

Re: pgjdbc is not working with PKCS8 certificates with password

[just madhu]

I see that the generated certificate is not working in pgAdmin and psql.
So I wanted a way by which I could make it work there as well.
As ANS.1 DER is a supported format for libpq, I suppose that this
certificate should work here as well.

Also as suggested checking in pgjdbc as well.

On Wed, Feb 7, 2024 at 8:22 PM Joe Conway  wrote:

> On 2/7/24 06:42, just madhu wrote:
> > On further investigation,
> > /With certificate generated as below. JDBC connection is successful./
> > openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out
> > client.pk8  -passout pass:foobar / -v1 PBE-MD5-DES
> >
> > But a connection from pgAdmin (connection failed:
> > \SSLCerts\pk8_pass\client_pass_PBE.pk8": no start line) and psql(psql:
> > error: could not load private key file "client_pass_PBE.pk8":
> > unsupported) is failing
> >
> > Is there a common way in which certificate with passwords can be
> > created  for both libpq and jdbc ?
>
>
> You may want to check with the pgjdbc project on github rather than (or
> in addition to?) here; see:
>
>https://github.com/pgjdbc/pgjdbc/issues
>
> Joe
>
> > On Wed, Feb 7, 2024 at 3:17 PM just madhu  > > wrote:
> >
> > Hi ,
> >
> > postgresql-42.7.1.jar
> >
> > Trying to use establish a connection using PKCS8 certificate created
> > with password.
> >
> > /openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out
> > client.pk8  -passout pass:foobar
> > /
> >
> > I set the properties as below:
> > /.../
> > /sslProperties.setProperty("sslkey", "client.pk8");
> > sslProperties.setProperty("sslpassword","foobar");/
> > /.../
> > /Connection connection = DriverManager.getConnection(jdbcUrl,
> > sslProperties);
> > /
> > //
> > /This is failing with the error:/
> > /org.postgresql.util.PSQLException: SSL error: Connection reset
> > at org.postgresql.ssl.MakeSSL.convert(MakeSSL.java:43)
> > at
> >
>  
> org.postgresql.core.v3.ConnectionFactoryImpl.enableSSL(ConnectionFactoryImpl.java:584)
> > at
> >
>  
> org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:168)
> > /
> > /.../
> >
> > Regards,
> > Madhu
> >
>
> --
> Joe Conway
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
>

Re: Psql meta-command conninfo+

[Pavel Luzanov]

Hi,Maiquel!


On 07.02.2024 17:47, Maiquel Grassi wrote:
"Also, it seems that the verbose parameter in the 
listConnectionInformation is unnecessary." Could you point out exactly 
the line or code snippet you are referring to?


+bool
+listConnectionInformation(const char *pattern,*bool verbose*)

I mean that parameter verbose is not used in the function body and 
listConnectionInformation called only in verbose mode.

--
Pavel Luzanov
Postgres Professional:https://postgrespro.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
>> when the required conditions (dependencies, options) are not there.
>> Maybe something like docs_pdf can serve as an example.
Makes sense, and it is simpler.
Let’s continue after I take a look at docs_pdf.

Thanks,
John

Combine headerscheck and cpluspluscheck scripts

[Peter Eisentraut]

headerscheck started in 55ea1091884 (2019) essentially as an adjusted 
copy of cpluspluscheck.  Since then two scripts have not drifted far 
apart.  But there are occasionally mistakes keeping the two exclude 
lists updated together.  I figure we can just combine the two scripts 
into one, so it's easier to keep updated.


The attached patch adds an option --cplusplus to headerscheck, with 
which it does the same thing as cpluspluscheck, and cpluspluscheck is 
removed.  The top-level make targets stay the same.From 097e135bc421af98dc6298510d4cb4d8d778e3fe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 7 Feb 2024 16:22:07 +0100
Subject: [PATCH] Combine headerscheck and cpluspluscheck scripts

They are mostly the same, and it is tedious to maintain two copies of
essentially the same exclude list.  headerscheck now has a new option
--cplusplus to select the cpluspluscheck functionality.  The top-level
make targets are still the same.
---
 GNUmakefile.in |   2 +-
 src/tools/pginclude/cpluspluscheck | 227 -
 src/tools/pginclude/headerscheck   |  57 +++-
 3 files changed, 54 insertions(+), 232 deletions(-)
 delete mode 100755 src/tools/pginclude/cpluspluscheck

diff --git a/GNUmakefile.in b/GNUmakefile.in
index eba569e930e..4d8fc794bbb 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -133,6 +133,6 @@ headerscheck: submake-generated-headers
$(top_srcdir)/src/tools/pginclude/headerscheck $(top_srcdir) 
$(abs_top_builddir)
 
 cpluspluscheck: submake-generated-headers
-   $(top_srcdir)/src/tools/pginclude/cpluspluscheck $(top_srcdir) 
$(abs_top_builddir)
+   $(top_srcdir)/src/tools/pginclude/headerscheck --cplusplus 
$(top_srcdir) $(abs_top_builddir)
 
 .PHONY: dist distdir distcheck docs install-docs world check-world 
install-world installcheck-world headerscheck cpluspluscheck
diff --git a/src/tools/pginclude/cpluspluscheck 
b/src/tools/pginclude/cpluspluscheck
deleted file mode 100755
index 7edfc44b49a..000
--- a/src/tools/pginclude/cpluspluscheck
+++ /dev/null
@@ -1,227 +0,0 @@
-#!/bin/sh
-
-# Check (almost) all PostgreSQL include files for C++ compatibility.
-#
-# Argument 1 is the top-level source directory, argument 2 the
-# top-level build directory (they might be the same). If not set, they
-# default to the current directory.
-#
-# Needs to be run after configuring and creating all generated headers.
-# It's advisable to configure --with-perl --with-python, else you're
-# likely to get errors from associated headers.
-#
-# No output if everything is OK, else compiler errors.
-#
-# src/tools/pginclude/cpluspluscheck
-# Copyright (c) 2009-2024, PostgreSQL Global Development Group
-
-if [ -z "$1" ]; then
-srcdir="."
-else
-srcdir="$1"
-fi
-
-if [ -z "$2" ]; then
-builddir="."
-else
-builddir="$2"
-fi
-
-me=`basename $0`
-
-# These switches are g++ specific, you may override if necessary.
-CXXFLAGS=${CXXFLAGS:- -fsyntax-only -Wall}
-
-# Pull some info from configure's results.
-MGLOB="$builddir/src/Makefile.global"
-CPPFLAGS=`sed -n 's/^CPPFLAGS[ ]*=[]*//p' "$MGLOB"`
-CXX=`sed -n 's/^CXX[   ]*=[]*//p' "$MGLOB"`
-perl_includespec=`sed -n 's/^perl_includespec[ ]*=[]*//p' "$MGLOB"`
-python_includespec=`sed -n 's/^python_includespec[ ]*=[]*//p' "$MGLOB"`
-
-# Extract any -I and -D switches from CPPFLAGS.
-# (If necessary, user can pass more switches by presetting EXTRAFLAGS.)
-for flag in $CPPFLAGS; do
-  case $flag in
--I*|-D*) EXTRAFLAGS="$EXTRAFLAGS $flag";;
-  esac
-done
-
-# Create temp directory.
-tmp=`mktemp -d /tmp/$me.XX`
-
-trap "ret=$?; rm -rf $tmp; exit $ret" 0 1 2 3 15
-
-exit_status=0
-
-# Scan all of src/ and contrib/ for header files.
-for f in `cd "$srcdir" && find src contrib -name '*.h' -print`
-do
-   # Ignore files that are unportable or intentionally not standalone.
-
-   # These files are platform-specific, and c.h will include the
-   # one that's relevant for our current platform anyway.
-   test "$f" = src/include/port/aix.h && continue
-   test "$f" = src/include/port/cygwin.h && continue
-   test "$f" = src/include/port/darwin.h && continue
-   test "$f" = src/include/port/freebsd.h && continue
-   test "$f" = src/include/port/linux.h && continue
-   test "$f" = src/include/port/netbsd.h && continue
-   test "$f" = src/include/port/openbsd.h && continue
-   test "$f" = src/include/port/solaris.h && continue
-   test "$f" = src/include/port/win32.h && continue
-
-   # Additional Windows-specific headers.
-   test "$f" = src/include/port/win32_port.h && continue
-   test "$f" = src/include/port/win32/netdb.h && continue
-   test "$f" = src/include/port/win32/sys/resource.h && continue
-   test "$f" = src/include/port/win32/sys/socket.h && continue
-   test "$f" = src/include/port/win32_msvc/dirent.h && continue
-   test "$f" = src/include/port/win32_msvc/utime.h &

Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability

[jian he]

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?

Re: Change GUC hashtable to use simplehash?

[Peter Eisentraut]

On 22.01.24 03:03, John Naylor wrote:

I wrote:

   fasthash_init(&hs, sizeof(Datum), kind);
   fasthash_accum(&hs, (char *) &value, sizeof(Datum));
   return fasthash_final32(&hs, 0);

It occurred to me that it's strange to have two places that length can
be passed. That was a side effect of the original, which used length
to both know how many bytes to read, and to modify the internal seed.
With the incremental API, it doesn't make sense to pass the length (or
a dummy macro) up front -- with a compile-time fixed length, it can't
possibly break a tie, so it's just noise.

0001 removes the length from initialization in the incremental
interface. The standalone functions use length directly the same as
before, but after initialization. Thoughts?


Unrelated related issue: src/include/common/hashfn_unstable.h currently 
causes warnings from cpluspluscheck:


/tmp/cirrus-ci-build/src/include/common/hashfn_unstable.h: In function 
‘int fasthash_accum_cstring_unaligned(fasthash_state*, const char*)’:
/tmp/cirrus-ci-build/src/include/common/hashfn_unstable.h:201:20: 
warning: comparison of integer expressions of different signedness: 
‘int’ and ‘long unsigned int’ [-Wsign-compare]

   201 |   while (chunk_len < FH_SIZEOF_ACCUM && str[chunk_len] != '\0')
   |^

and a few more like that.

I think it would be better to declare various int variables and 
arguments as size_t instead.  Even if you don't actually need the larger 
range, it would make it more self-documenting.

Re: Printing backtrace of postgres processes

[Bharath Rupireddy]

On Wed, Feb 7, 2024 at 2:57 PM Michael Paquier  wrote:
>
> On Wed, Feb 07, 2024 at 02:04:39PM +0530, Bharath Rupireddy wrote:
> > Well, that 'ubuntu' is the default username there, I've changed it now
> > and kept the output short.
>
> I would keep it just at two or three lines, with a "For example, with
> lines like":

Done.

> > I've simplified the tests, now we don't need two separate output files
> > for tests. Please see the attached v27 patch.
>
> +  proname => 'pg_log_backtrace', provolatile => 'v', prorettype => 'bool',
>
> Hmm.  Would it be better to be in line with memory contexts logging
> and use pg_log_backend_backtrace()?

+1.

> One thing I was wondering is that
> there may be a good point in splitting the backtrace support into two
> functions (backends and auxiliary processes) that could be split with
> two execution ACLs across different roles.

-1 for that unless we have any requests. I mean, backtrace is a common
thing for all postgres processes, why different controls are needed?
I'd go with what pg_log_backend_memory_contexts does - it supports
both backends and auxiliary processes.

> +   PROCSIG_LOG_BACKTRACE,  /* ask backend to log the current backtrace */
>
> Incorrect order.

PROCSIG_XXX aren't alphabetically ordered, no?

> +-- Backtrace is not logged for auxiliary processes
>
> Not sure there's a point in keeping that in the tests for now.
>
> +* XXX: It might be worth implementing it for auxiliary processes.
>
> Same, I would remove that.

Done.

> +static volatile sig_atomic_t backtrace_functions_loaded = false;
>
> Hmm, so you need that because of the fact that it would be called in a
> signal as backtrace(3) says:
> "If you need certain calls to these two functions to not allocate
> memory (in signal handlers, for example), you need to make sure libgcc
> is loaded beforehand".
>
> True that it is not interesting to only log something when having a
> CFI, this needs to be dynamic to get a precise state of things.

Right.

I've also fixed some test failures. Please see the attached v28 patch
set. 0002 extends pg_log_backend_backtrace to auxiliary processes,
just like pg_log_backend_memory_contexts (not focused on PID
de-duplication code yet).

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


v28-0002-Extend-backtrace-logging-function-to-auxiliary-p.patch
Description: Binary data


v28-0001-Add-function-to-log-backtrace-of-a-backend.patch
Description: Binary data

Re: pg_get_expr locking

[Tom Lane]

Peter Eisentraut  writes:
> The function pg_get_expr(), which is used in various system views and 
> information schema views, does not appear to lock the table passed as 
> the second argument, and so appears to be liable to fail if there is a 
> concurrent drop of the table.  There is a (probable) case of this being 
> discussed at [0].  I also see various mentions of this issue in the 
> commit logs, mostly related to pg_dump.

> Is there a reason there is no locking?  Performance?

I think we have a general rule that you shouldn't be able to take
locks on relations you have no privileges for, so pg_get_expr would
need to verify privileges if it locked the rel.  At least for
pg_dump's purposes, that cure would be about as bad as the disease.

> 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?

regards, tom lane

Re: Set log_lock_waits=on by default

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> >> On Tue, 2024-02-06 at 12:29 -0500, Tom Lane wrote:
> >>> I was, and remain, of the opinion that that was a bad idea that
> >>> we'll eventually revert, just like we previously got rid of most
> >>> inessential log chatter in the default configuration.
> 
> >> Unsurprisingly, I want to argue against that.
> 
> > I tend to agree with this position- log_checkpoints being on has been a
> > recommended configuration for a very long time and is valuable
> > information to have about what's been happening when someone does go and
> > look at the log.
> 
> We turned on default log_checkpoints in v15, which means that behavior
> has been in the field for about sixteen months.  I don't think that
> that gives it the status of a settled issue; my bet is that most
> users still have not seen it.

Apologies for not being clear- log_checkpoints being on has been a
configuration setting that I (and many others I've run into) have been
recommending since as far back as I can remember.

I was not referring to the change in the default.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Set log_lock_waits=on by default

[Tom Lane]

Stephen Frost  writes:
>> On Tue, 2024-02-06 at 12:29 -0500, Tom Lane wrote:
>>> I was, and remain, of the opinion that that was a bad idea that
>>> we'll eventually revert, just like we previously got rid of most
>>> inessential log chatter in the default configuration.

>> Unsurprisingly, I want to argue against that.

> I tend to agree with this position- log_checkpoints being on has been a
> recommended configuration for a very long time and is valuable
> information to have about what's been happening when someone does go and
> look at the log.

We turned on default log_checkpoints in v15, which means that behavior
has been in the field for about sixteen months.  I don't think that
that gives it the status of a settled issue; my bet is that most
users still have not seen it.

regards, tom lane

Re: pgjdbc is not working with PKCS8 certificates with password

[just madhu]

On further investigation,

*With certificate generated as below. JDBC connection is successful.openssl
pkcs8 -topk8 -inform PEM -in client.key -outform DER -out client.pk8
 -passout pass:foobar  * -v1 PBE-MD5-DES

But a connection from pgAdmin (connection failed:
\SSLCerts\pk8_pass\client_pass_PBE.pk8": no start line) and psql(psql:
error: could not load private key file "client_pass_PBE.pk8": unsupported)
is failing

Is there a common way in which certificate with passwords can be created
for both libpq and jdbc ?


On Wed, Feb 7, 2024 at 3:17 PM just madhu  wrote:

> Hi ,
>
> postgresql-42.7.1.jar
>
> Trying to use establish a connection using PKCS8 certificate created with
> password.
>
>
> *openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out
> client.pk8  -passout pass:foobar*
>
> I set the properties as below:
> *...*
>
> *sslProperties.setProperty("sslkey",
> "client.pk8");sslProperties.setProperty("sslpassword","foobar");*
> *...*
>
> *Connection connection = DriverManager.getConnection(jdbcUrl,
> sslProperties);*
> **
> *This is failing with the error:*
>
>
>
>
> *org.postgresql.util.PSQLException: SSL error: Connection reset at
> org.postgresql.ssl.MakeSSL.convert(MakeSSL.java:43) at
> org.postgresql.core.v3.ConnectionFactoryImpl.enableSSL(ConnectionFactoryImpl.java:584)
> at
> org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:168)*
> *...*
>
> Regards,
> Madhu
>
>

Re: pgjdbc is not working with PKCS8 certificates with password

[Joe Conway]

On 2/7/24 06:42, just madhu wrote:

On further investigation,
/With certificate generated as below. JDBC connection is successful./
openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out 
client.pk8  -passout pass:foobar / -v1 PBE-MD5-DES


But a connection from pgAdmin (connection failed: 
\SSLCerts\pk8_pass\client_pass_PBE.pk8": no start line) and psql(psql: 
error: could not load private key file "client_pass_PBE.pk8": 
unsupported) is failing


Is there a common way in which certificate with passwords can be 
created  for both libpq and jdbc ?



You may want to check with the pgjdbc project on github rather than (or 
in addition to?) here; see:


  https://github.com/pgjdbc/pgjdbc/issues

Joe

On Wed, Feb 7, 2024 at 3:17 PM just madhu > wrote:


Hi ,

postgresql-42.7.1.jar

Trying to use establish a connection using PKCS8 certificate created
with password.

/openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out
client.pk8  -passout pass:foobar
/

I set the properties as below:
/.../
/sslProperties.setProperty("sslkey", "client.pk8");
sslProperties.setProperty("sslpassword","foobar");/
/.../
/Connection connection = DriverManager.getConnection(jdbcUrl,
sslProperties);
/
//
/This is failing with the error:/
/org.postgresql.util.PSQLException: SSL error: Connection reset
at org.postgresql.ssl.MakeSSL.convert(MakeSSL.java:43)
at

org.postgresql.core.v3.ConnectionFactoryImpl.enableSSL(ConnectionFactoryImpl.java:584)
at

org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:168)
/
/.../

Regards,
Madhu



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

RE: Psql meta-command conninfo+

[Maiquel Grassi]

  This is a good idea about extended connection info.

  On 07.02.2024 07:13, Maiquel Grassi wrote:
SELECT
...
  current_user AS "User",

  This will be inconsistent with \conninfo.
  \conninfo returns authenticated user (PQuser), not the current_user.
  It might be worth showing current_user, session_user, and authenticated user,
  but I can't find the appropriate sql function for PQuser.

  What about to include system_user function? It shows useful authentication 
details.

  Also, it seems that the verbose parameter in the listConnectionInformation
  is unnecessary.

--//--

Hi,

Tks Pavel.

Analyzing the functions' code more thoroughly, it seems to make more sense.
I liked your suggestions and implemented them for validation.
Regarding "system_user," I believe it is valid and also added it to the row.

"Also, it seems that the verbose parameter in the listConnectionInformation is 
unnecessary."
Could you point out exactly the line or code snippet you are referring to?

To print the string from the "Authenticated User" column, I chose to use 
PQuser(pset.db) directly. I did the same for the "Host" column, opting for 
PQhost(pset.db). This does not contradict the result of \conninfo.

Here are the tests as usual, and v6 patch.

[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 | Current User | Session User | System User | 
Server Version | Server Address | Server Port | Client Address | Client Port | 
Session PID | Socket Directory | Host
--++--+--+-+++-++-+-+--+--
 postgres | postgres   | postgres | postgres | | 
17devel|| 5432|| |  
 17240 | /tmp |
(1 row)

postgres=# \q
[postgres@localhost bin]$ ./psql -h ::1
psql (17devel)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" on host "::1" at 
port "5432".
postgres=# \conninfo+

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

postgres=# \q
[postgres@localhost bin]$ ./psql -h 127.0.0.1
psql (17devel)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" on host "127.0.0.1" 
at port "5432".
postgres=# \conninfo+

Current Connection Information
 Database | Authenticated User | Current User | Session User | System User | 
Server Version | Server Address | Server Port | Client Address | Client Port | 
Session PID | Socket Directory |   Host
--++--+--+-+++-++-+-+--+---
 postgres | postgres   | postgres | postgres | | 
17devel| 127.0.0.1  | 5432| 127.0.0.1  |   35076 |  
 17245 | /tmp | 127.0.0.1
(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 | Current User | Session User | System User | 
Server Version | Server Address | Server Port | Client Address | Client Port | 
Session PID | Socket Directory |   Host
--++--+--+-+++-++-+-+--+---
 postgres | postgres   | postgres | postgres | | 
17devel| ::1| 5432| ::1|   47028 |  
 17

Re: Set log_lock_waits=on by default

[Stephen Frost]

Greetings,

* Laurenz Albe (laurenz.a...@cybertec.at) wrote:
> On Tue, 2024-02-06 at 12:29 -0500, Tom Lane wrote:
> > Nathan Bossart  writes:
> > > It looks like there are two ideas:
> > > [...]
> > > * Set log_lock_waits on by default.  The folks on this thread seem to
> > >   support this idea, but given the lively discussion for enabling
> > >   log_checkpoints by default [0], I'm hesitant to commit something like
> > >   this without further community discussion.
> > 
> > I was, and remain, of the opinion that that was a bad idea that
> > we'll eventually revert, just like we previously got rid of most
> > inessential log chatter in the default configuration.  So I doubt
> > you'll have much trouble guessing my opinion of this one.  I think
> > the default logging configuration should be chosen with the
> > understanding that nobody ever looks at the logs of most
> > installations, and we should be more worried about their disk space
> > consumption than anything else.  Admittedly, log_lock_waits is less
> > bad than log_checkpoints, because no such events will occur in a
> > well-tuned configuration ... but still, it's going to be useless
> > chatter in the average installation.
> 
> Unsurprisingly, I want to argue against that.

I tend to agree with this position- log_checkpoints being on has been a
recommended configuration for a very long time and is valuable
information to have about what's been happening when someone does go and
look at the log.

Having log_lock_waits on by default is likely to be less noisy and even
more useful for going back in time to figure out what happened.

> You say that it is less bad than "log_checkpoints = on", and I agree.
> I can't remember seeing any complaints by users about
> "log_checkpoints", and I predict that the complaints about
> "log_lock_waits = on" will be about as loud.

Yeah, agreed.

> I am all for avoiding useless chatter in the log.  In my personal
> experience, that is usually "database typo does not exist" and
> constraint violation errors.  I always recommend people to enable
> "log_lock_waits", and so far I have not seen it spam the logs.

I really wish we could separate out the messages about typos and
constraint violations from these logs about processes waiting a long
time for locks or about checkpoints or even PANIC's or other really
important messages.  That said, that's a different problem and not
something this change needs to concern itself with.

As for if we want to separate out log_lock_waits from deadlock_timeout-
no, I don't think we do, for the reasons that Tom mentioned.  I don't
see it as necessary either for enabling log_lock_waits by default.
Waiting deadlock_timeout amount of time for a lock certainly is a
problem already and once we've waited that amount of time, I can't see
the time spent logging about it as being a serious issue.

+1 for simply enabling log_lock_waits by default.

All that said ... if we did come up with a nice way to separate out the
timing for deadlock_timeout and log_lock_waits, I wouldn't necessarily
be against it.  Perhaps one approach to that would be to set just one
timer but have it be the lower of the two, and then set another when
that fires (if there's more waiting to do) and then catch it when it
happens...  Again, I'd view this as some independent improvement though
and not a requirement for just enabling log_lock_waits by default.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Commitfest 2024-01 first week update

[Daniel Gustafsson]

> On 7 Feb 2024, at 14:15, Alvaro Herrera  wrote:
> 
> On 2024-Feb-07, Daniel Gustafsson wrote:
> 
>> Since the CF app knows when the last email in the thread was, the
>> state of the patch entry and the number of CF's which is has been
>> present in; maybe we can extend the app to highlight these patches in
>> a way which doesn't add more manual intervention?  It might yield a
>> few false positives, but so will setting it manually.
> 
> Hmm, but suppose the author is posting new versions now and again
> because of apply conflicts; and the CFM is pinging them about that, but
> not providing any actionable feedback.  Such a thread would be active
> enough, but the patch is still stalled.

If the patch author is actively rebasing the patch and thus generating traffic
on the thread I'm not sure I would call it stalled - there might not be enough
(or any) reviews but the activity alone might make someone interested.  Either
way, I'd personally prefer such false positives if it means reducing the manual
workload for the CFM.

--
Daniel Gustafsson

What about Perl autodie?

[Peter Eisentraut]

I came across the Perl autodie pragma 
(https://perldoc.perl.org/autodie).  This seems pretty useful; is this 
something we can use?  Any drawbacks?  Any minimum Perl version?


Attached is a sample patch of the kind of thing I'd be interested in. 
The existing error handling of file operations in Perl is pretty 
cumbersome, and this would simplify that.


Btw., here is a sample error message from autodie:

Can't open '../src/include/mb/pg_wchar.h' for reading: 'No such file or 
directory' at ../src/include/catalog/../../backend/catalog/genbki.pl 
line 391


which seems as good or better than the stuff we produce manually.From 4ada07b13ecde8b3c0d120583202a38de062239f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 7 Feb 2024 14:59:36 +0100
Subject: [PATCH] WIP: Make some use of Perl autodie pragma

---
 src/backend/catalog/Catalog.pm | 11 ++-
 src/backend/catalog/genbki.pl  | 24 +---
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 55a8877aede..fa42d472df1 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -15,6 +15,7 @@ package Catalog;
 
 use strict;
 use warnings FATAL => 'all';
+use autodie;
 
 use File::Compare;
 
@@ -48,7 +49,7 @@ sub ParseHeader
$catalog{foreign_keys} = [];
$catalog{client_code} = [];
 
-   open(my $ifh, '<', $input_file) || die "$input_file: $!";
+   open(my $ifh, '<', $input_file);
 
# Scan the input file.
while (<$ifh>)
@@ -307,7 +308,7 @@ sub ParseData
 {
my ($input_file, $schema, $preserve_formatting) = @_;
 
-   open(my $ifd, '<', $input_file) || die "$input_file: $!";
+   open(my $ifd, '<', $input_file);
$input_file =~ /(\w+)\.dat$/
  or die "Input file $input_file needs to be a .dat file.\n";
my $catname = $1;
@@ -531,11 +532,11 @@ sub RenameTempFile
if (-f $final_name
&& compare($temp_name, $final_name) == 0)
{
-   unlink($temp_name) || die "unlink: $temp_name: $!";
+   unlink($temp_name);
}
else
{
-   rename($temp_name, $final_name) || die "rename: $temp_name: $!";
+   rename($temp_name, $final_name);
}
return;
 }
@@ -553,7 +554,7 @@ sub FindDefinedSymbol
$include_path .= '/';
}
my $file = $include_path . $catalog_header;
-   open(my $find_defined_symbol, '<', $file) || die "$file: $!";
+   open(my $find_defined_symbol, '<', $file);
while (<$find_defined_symbol>)
{
if (/^#define\s+\Q$symbol\E\s+(\S+)/)
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 94afdc5491d..dc8cba037fd 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -15,6 +15,7 @@
 
 use strict;
 use warnings FATAL => 'all';
+use autodie;
 use Getopt::Long;
 
 use FindBin;
@@ -386,7 +387,7 @@
 my %encids;
 
 my $encfile = $include_path . 'mb/pg_wchar.h';
-open(my $ef, '<', $encfile) || die "$encfile: $!";
+open(my $ef, '<', $encfile);
 
 # We're parsing an enum, so start with 0 and increment
 # every time we find an enum member.
@@ -435,23 +436,17 @@
 # Open temp files
 my $tmpext = ".tmp$$";
 my $bkifile = $output_path . 'postgres.bki';
-open my $bki, '>', $bkifile . $tmpext
-  or die "can't open $bkifile$tmpext: $!";
+open my $bki, '>', $bkifile . $tmpext;
 my $schemafile = $output_path . 'schemapg.h';
-open my $schemapg, '>', $schemafile . $tmpext
-  or die "can't open $schemafile$tmpext: $!";
+open my $schemapg, '>', $schemafile . $tmpext;
 my $fk_info_file = $output_path . 'system_fk_info.h';
-open my $fk_info, '>', $fk_info_file . $tmpext
-  or die "can't open $fk_info_file$tmpext: $!";
+open my $fk_info, '>', $fk_info_file . $tmpext;
 my $constraints_file = $output_path . 'system_constraints.sql';
-open my $constraints, '>', $constraints_file . $tmpext
-  or die "can't open $constraints_file$tmpext: $!";
+open my $constraints, '>', $constraints_file . $tmpext;
 my $syscache_ids_file = $output_path . 'syscache_ids.h';
-open my $syscache_ids_fh, '>', $syscache_ids_file . $tmpext
-  or die "can't open $syscache_ids_file$tmpext: $!";
+open my $syscache_ids_fh, '>', $syscache_ids_file . $tmpext;
 my $syscache_info_file = $output_path . 'syscache_info.h';
-open my $syscache_info_fh, '>', $syscache_info_file . $tmpext
-  or die "can't open $syscache_info_file$tmpext: $!";
+open my $syscache_info_fh, '>', $syscache_info_file . $tmpext;
 
 # Generate postgres.bki and pg_*_d.h headers.
 
@@ -469,8 +464,7 @@
 
# Create one definition header with macro definitions for each catalog.
my $def_file = $output_path . $catname . '_d.h';
-   open my $def, '>', $def_file . $tmpext
- or die "can't open $def_file$tmpext: $!";
+   open my $def, '>', $def_file . $tmpext;
 
print_boilerplate($def, "${catname

Re: Commitfest 2024-01 first week update

[Amit Kapila]

On Wed, Feb 7, 2024 at 6:07 PM Alvaro Herrera  wrote:
>
> On 2024-Feb-04, vignesh C wrote:
>
> > We should do something about these kinds of entries, there were few
> > suggestions like tagging under a new category or so, can we add a new
> > status to park these entries something like "Waiting for direction".
> > The threads which have no discussion for 6 months or so can be flagged
> > to this new status and these can be discussed in one of the developer
> > meetings or so and conclude on these items.
>
> Maybe a new status is appropriate ... I would suggest "Stalled".  Such a
> patch still applies and has no pending feedback, but nobody seems
> interested.  Making a patch no longer stalled means there's discussion
> that leads to:
>
> 1. further development?  Perhaps the author just needed more direction.
> 2. a decision that it's not a feature we want, or maybe not in this
>form.  Then we close it as rejected.
> 3. a reviewer/committer finding time to provide additional feedback.
>

+1. This suggestion sounds reasonable. I think we probably need to
decide the time when the patch's status should be changed to
"Stalled".

-- 
With Regards,
Amit Kapila.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 7 Feb 2024 at 11:35, Gabriele Bartolini
 wrote:
> This is mostly the approach I have taken in the patch, except allowing to 
> change the value in the configuration file.

(I had missed the patch in the long thread). I think it would be nice
to have this be PGC_SIGHUP, and set GUC_DISALLOW_IN_AUTO_FILE. That
way this behaviour can be changed without shutting down postgres (but
not with ALTER SYSTEM, because that seems confusing).

> but wasn't sure in which `config_group` to place the 'enable_alter_system` 
> GUC, based on the src/include/utils/guc_tables.h. Any thoughts/hints?

I agree that none of the existing groups fit particularly well. I see
a few options:

1. Create a new group (maybe something like "Administration" or
"Enabled Features")
2. Use FILE_LOCATIONS, which seems sort of related at least.
3. Instead of adding an "enable_alter_system" GUC we would add an
"auto_config_file" guc (and use the FILE_LOCATIONS group). Then if a
user sets "auto_config_file" to an empty string, we would disable the
auto config file and thus ALTER SYSTEM.

I'd prefer 1 or 3 I think. I kinda like option 3 for its consistency
of being able to configure other config file locations, but I think
that would be quite a bit more work, and I'm not sure how useful it is
to change the location of the auto file.

Re: Possibility to disable `ALTER SYSTEM`

[David G. Johnston]

On Wednesday, February 7, 2024, Joel Jacobson  wrote:

>
> On Fri, Sep 8, 2023, at 23:43, Magnus Hagander wrote:
> > We need a "allowlist" of things a user can do, rather than a blocklist
> > of "they can do everything they can possibly think of and a computer
> > is capable of doing, except for this one specific thing". Blocklisting
> > individual permissions of a superuser will never be secure.
>
> +1 for preferring an "allowlist" approach over a blocklist.
>

The status quo is allow everything so while the theory is nice it seems
that requiring it to be allowlist is just going to scare anyone off of
actually improving matters.

Also, this isn’t necessarily about blocking the superuser, it is about
effectively disabling features deemed undesirable at runtime.  All features
enabled by default seems like a valid policy.

While the only features likely to be disabled are those involving someone’s
definition of security the security policy is still that superuser can do
everything the system is capable of doing.

David J.

Re: Possibility to disable `ALTER SYSTEM`

[Jelte Fennema-Nio]

On Wed, 7 Feb 2024 at 11:16, Peter Eisentraut  wrote:
> On 06.02.24 16:22, Jelte Fennema-Nio wrote:
> > On Tue, 30 Jan 2024 at 18:49, Robert Haas  wrote:
> >> I also think that using the GUC system to manage itself is a little
> >> bit suspect. I wonder if it would be better to do this some other way,
> >> e.g. a sentinel file in the data directory. For example, suppose we
> >> refuse ALTER SYSTEM if $PGDATA/disable_alter_system exists, or
> >> something like that.
> > I'm not convinced we need a new file to disable ALTER SYSTEM. I feel
> > like an "enable_alter_system" GUC that defaults to ON would work fine
> > for this. If we make that GUC be PGC_POSTMASTER then an operator can
> > disable ALTER SYSTEM either with a command line argument or by
> > changing the main config file. Since this feature is mostly useful
> > when the config file is managed by an external system, it seems rather
> > simple for that system to configure one extra GUC in the config file.
>
> Yeah, I'm all for that, but some others didn't like handling this in the
> GUC system, so I'm throwing around other ideas.

Okay, then we're agreeing here. Reading back the email thread the only
argument against GUCs that I could find was Robert thinking it is a "a
little bit suspect" to let the GUC system manage itself. This would
not be the first time we're doing that though, the same is true for
"config_file" and "data_directory" (which even needed the introduction
of GUC_DISALLOW_IN_AUTO_FILE).

So, I personally would like to hear some other options before we start
entertaining some new ways of configuring Postgres its behaviour (even
the read-only postgresql.auto.conf seems quite strange to me).

Re: scram_iterations is undocumented GUC_REPORT

[Daniel Gustafsson]

> On 30 Jan 2024, at 15:44, Alvaro Herrera  wrote:

> I propose to turn the list into a
> 
> which looks _much_ nicer to read, as in the attached screenshot of the
> PDF.

+1, this reads a lot better.

--
Daniel Gustafsson

Re: Reducing connection overhead in pg_upgrade compat check phase

[Daniel Gustafsson]

> 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.

--
Daniel Gustafsson



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

Re: Commitfest 2024-01 first week update

[Alvaro Herrera]

On 2024-Feb-07, Daniel Gustafsson wrote:

> Since the CF app knows when the last email in the thread was, the
> state of the patch entry and the number of CF's which is has been
> present in; maybe we can extend the app to highlight these patches in
> a way which doesn't add more manual intervention?  It might yield a
> few false positives, but so will setting it manually.

Hmm, but suppose the author is posting new versions now and again
because of apply conflicts; and the CFM is pinging them about that, but
not providing any actionable feedback.  Such a thread would be active
enough, but the patch is still stalled.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No hay hombre que no aspire a la plenitud, es decir,
la suma de experiencias de que un hombre es capaz"

Re: Why is subscription/t/031_column_list.pl failing so much?

[vignesh C]

On Wed, 7 Feb 2024 at 15:26, Amit Kapila  wrote:
>
> On Wed, Feb 7, 2024 at 2:06 AM Tom Lane  wrote:
> >
> > I wrote:
> > > More to the point, aren't these proposals just band-aids that
> > > would stabilize the test without fixing the actual problem?
> > > The same thing is likely to happen to people in the field,
> > > unless we do something drastic like removing ALTER SUBSCRIPTION.
> >
> > I've been able to make the 031_column_list.pl failure pretty
> > reproducible by adding a delay in walsender, as attached.
> >
> > While I'm not too familiar with this code, it definitely does appear
> > that the new walsender is told to start up at an LSN before the
> > creation of the publication, and then if it needs to decide whether
> > to stream a particular data change before it's reached that creation,
> > kaboom!
> >
> > I read and understood the upthread worries about it not being
> > a great idea to ignore publication lookup failures, but I really
> > don't see that we have much choice.  As an example, if a subscriber
> > is humming along reading publication pub1, and then someone
> > drops and then recreates pub1 on the publisher, I don't think that
> > the subscriber will be able to advance through that gap if there
> > are any operations within it that require deciding if they should
> > be streamed.
> >
>
> Right. One idea to address those worries was to have a new
> subscription option like ignore_nonexistant_pubs (or some better name
> for such an option). The 'true' value of this new option means that we
> will ignore the publication lookup failures and continue replication,
> the 'false' means give an error as we are doing now. If we agree that
> such an option is useful or at least saves us in some cases as
> discussed in another thread [1], we can keep the default value as true
> so that users don't face such errors by default and also have a way to
> go back to current behavior.
>
> >
>   (That is, contrary to Amit's expectation that
> > DROP/CREATE would mask the problem, I suspect it will instead turn
> > it into a hard failure.  I've not experimented though.)
> >
>
> This is not contrary because I was suggesting to DROP/CREATE
> Subscription whereas you are talking of drop and recreate of
> Publication.
>
> > BTW, this same change breaks two other subscription tests:
> > 015_stream.pl and 022_twophase_cascade.pl.
> > The symptoms are different (no "publication does not exist" errors),
> > so maybe these are just test problems not fundamental weaknesses.
> >
>
> As per the initial analysis, this is because those cases have somewhat
> larger transactions (more than 64kB) under test so it just times out
> waiting for all the data to be replicated. We will do further analysis
> and share the findings.

Yes, these tests are failing while waiting to catchup the larger
transactions to be replicated within180 seconds, as the transactions
needs more time to replicate because of the sleep added. To verify
this I had tried a couple of things a) I had increased the timeout to
a higher value and verified that both the test runs successfully with
1800 seconds timeout. b) I reduced the sleep to 1000 microseconds and
verified that both the test runs successfully.

So I feel these tests 015_stream.pl and 022_twophase_cascade.pl
failing after the sleep added can be ignored.

Regards,
Vignesh

Re: Commitfest 2024-01 first week update

[Daniel Gustafsson]

> On 7 Feb 2024, at 13:37, Alvaro Herrera  wrote:

> Maybe a new status is appropriate ... I would suggest "Stalled".  Such a
> patch still applies and has no pending feedback, but nobody seems
> interested.

Since the CF app knows when the last email in the thread was, the state of the
patch entry and the number of CF's which is has been present in; maybe we can
extend the app to highlight these patches in a way which doesn't add more
manual intervention?  It might yield a few false positives, but so will setting
it manually.

--
Daniel Gustafsson

RE: Psql meta-command conninfo+

[Maiquel Grassi]

  On 2024-02-07 05:13 +0100, Maiquel Grassi wrote:
  >   On Tue, Feb 06, 2024 at 09:45:54PM +, Maiquel Grassi wrote:
  >   > My initial idea has always been that they should continue to appear
  >   > because \conninfo+ should show all the things that \conninfo shows and
  >   > add more information. I think that's the purpose of the 'plus.' Now 
we're
  >   > on a better path than the initial one. We can still add the socket
  >   > directory and the host.
  >
  >   Agreed.
  >
  > --//--
  >
  > I believe it's resolved reasonably well this way:
  >
  > SELECT
  >   pg_catalog.current_database() AS "Database",
  >   current_user AS "User",
  >   pg_catalog.current_setting('server_version') AS "Server Version",
  >   CASE
  > WHEN pg_catalog.inet_server_addr() IS NULL
  > THEN 'NULL'
  > ELSE pg_catalog.inet_server_addr()::text
  >   END AS "Server Address",

  Should be NULL instead of string 'NULL'.  So the entire CASE expression
  is redundant and you can just return pg_catalog.inet_server_addr().

  >   pg_catalog.current_setting('port') AS "Port",
  >   CASE
  > WHEN pg_catalog.inet_client_addr() IS NULL
  > THEN 'NULL'
  > ELSE pg_catalog.inet_client_addr()::text
  >   END AS "Client Address",
  >   CASE
  > WHEN pg_catalog.inet_client_port() IS NULL
  >  THEN 'NULL'
  > ELSE pg_catalog.inet_client_port()::text
  >   END AS "Client Port",

  Same here.

  >   pg_catalog.pg_backend_pid() AS "Session PID",
  >   CASE
  > WHEN pg_catalog.current_setting('unix_socket_directories') = ''
  > THEN 'NULL'
  > ELSE pg_catalog.current_setting('unix_socket_directories')
  >   END AS "Socket Directory",

  The CASE expression can be simplified to:

  nullif(pg_catalog.current_setting('unix_socket_directories'), '')

  >   CASE
  > WHEN
  >   pg_catalog.inet_server_addr() IS NULL
  >   AND pg_catalog.inet_client_addr() IS NULL
  > THEN 'NULL'
  > WHEN
  >   pg_catalog.inet_server_addr() = pg_catalog.inet_client_addr()
  > THEN 'localhost'

  Is it safe to assume localhost here?  \conninfo prints localhost only
  when I connect with psql -hlocalhost:

  $ psql -hlocalhost postgres
  psql (16.1)
  postgres=# \conninfo
  You are connected to database "postgres" as user "ewie" on host 
"localhost" (address "::1") at port "5432".
  postgres=# \q

  $ psql -h127.0.0.1 postgres
  psql (16.1)
  postgres=# \conninfo
  You are connected to database "postgres" as user "ewie" on host 
"127.0.0.1" at port "5432".

  > ELSE pg_catalog.inet_server_addr()::text
  >   END AS "Host";

--//--

There really was no need for the CASES. However, they helped visualize the psql 
output since for the null value, no word is printed on the screen. I made the 
adjustment by removing this redundancy.

Regarding the "Host" column, the most reliable way to solve this, I believe, is 
by using the "host" variable. So it's necessary to declare char *host = 
PQhost(pset.db); in listConnectionInformation() and use it in the SQL (see 
patch v5). This way, we have the same return from \conninfo reliably.

Once again, I ran a series of tests.

[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 |   User   | Server Version | Server Address | Server Port | Client 
Address | Client Port | Session PID | Socket Directory | Host
--+--+++-++-+-+--+--
 postgres | postgres | 17devel|| 5432|  
  | |   15898 | /tmp |
(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 |   User   | Server Version | Server Address | Server Port | Client 
Address | Client Port | Session PID | Socket Directory |   Host
--+--+++-++-+-+--+---
 postgres | postgres | 17devel| ::1| 5432| ::1  
  |   47012 |   15900 | /tmp | localhost
(1 row)

postgres=# \q
[postgres@localhost bin]$ ./psql -h ::1
psql (17devel)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" on host "::1" at 
port "5432".
postgres=# \conninfo+
   

Re: Commitfest 2024-01 first week update

[Alvaro Herrera]

On 2024-Feb-04, vignesh C wrote:

> We should do something about these kinds of entries, there were few
> suggestions like tagging under a new category or so, can we add a new
> status to park these entries something like "Waiting for direction".
> The threads which have no discussion for 6 months or so can be flagged
> to this new status and these can be discussed in one of the developer
> meetings or so and conclude on these items.

Maybe a new status is appropriate ... I would suggest "Stalled".  Such a
patch still applies and has no pending feedback, but nobody seems
interested.  Making a patch no longer stalled means there's discussion
that leads to:

1. further development?  Perhaps the author just needed more direction.
2. a decision that it's not a feature we want, or maybe not in this
   form.  Then we close it as rejected.
3. a reviewer/committer finding time to provide additional feedback.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)

Re: Psql meta-command conninfo+

[Pavel Luzanov]

This is a good idea about extended connection info.

On 07.02.2024 07:13, Maiquel Grassi wrote:

SELECT
...
current_user AS "User",


This will be inconsistent with \conninfo.
\conninfo returns authenticated user (PQuser), not the current_user.
It might be worth showing current_user, session_user, and authenticated user,
but I can't find the appropriate sql function for PQuser.

What about to include system_user function? It shows useful authentication 
details.

Also, it seems that the verbose parameter in the listConnectionInformation
is unnecessary.

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

Re: Synchronizing slots from primary to standby

[Dilip Kumar]

> > So now if we have such a functionality then it would be even better to
> > extend it to selectively sync the slot.  For example, if there is some
> > issue in syncing all slots, maybe some bug or taking a long time to
> > sync because there are a lot of slots but if the user needs to quickly
> > failover and he/she is interested in only a couple of slots then such
> > a option could be helpful. no?
> >
>
> I see your point but not sure how useful it is in the field. I am fine
> if others also think such a parameter will be useful and anyway I
> think we can even extend it after v1 is done.
>

Okay, I am fine with that.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com