small documentation fixes related to collations/ICU

[Peter Eisentraut]

I found two mistakes related to collation and/or ICU support in the 
documentation that should probably be fixed and backpatched.  See 
attached patches.From 44ea0d75f2739b6a3eed9a0233c3dcb2a64b2538 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 29 Apr 2024 08:50:20 +0200
Subject: [PATCH 1/2] doc: Fix description of --with-icu option

It was claiming that the ICU locale provider is used by default, which
is not correct.  (From commit fcb21b3acdc; it was once contemplated to
make it the default, but it wouldn't have been part of that patch in
any case.)

TODO backpatch 16
---
 doc/src/sgml/installation.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index a453f804cd6..1b32d5ca62c 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -167,7 +167,7 @@ Requirements
 
 
  
-  The ICU locale provider (see ) is used 
by default. If you don't want to use it then you must specify the 
--without-icu option to configure. Using 
this option disables support for ICU collation features (see ).
+  The ICU library is used by default. If you don't want to use it then you 
must specify the --without-icu option to 
configure. Using this option disables support for ICU 
collation features (see ).
  
  
   ICU support requires the ICU4C package to be

base-commit: 5c9f35fc48ea99e59300a267e090e3eafd1b3b0e
-- 
2.44.0

From c2a2fdd1272b24f2513e18f199370491b848c1b9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 29 Apr 2024 08:55:45 +0200
Subject: [PATCH 2/2] doc: Fix description of deterministic flag of CREATE
 COLLATION

The documentation said that you need to pick a suitable LC_COLLATE
setting in addition to setting the DETERMINISTIC flag.  This would
have been correct if the libc provider supported nondeterministic
collations, but since it doesn't, you actually need to set the LOCALE
option.

TODO backpatch 12
---
 doc/src/sgml/ref/create_collation.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/create_collation.sgml 
b/doc/src/sgml/ref/create_collation.sgml
index 85f18cbbe5d..e34bfc97c3d 100644
--- a/doc/src/sgml/ref/create_collation.sgml
+++ b/doc/src/sgml/ref/create_collation.sgml
@@ -154,7 +154,7 @@ Parameters
logically equal by the comparison.  PostgreSQL breaks ties using a
byte-wise comparison.  Comparison that is not deterministic can make the
collation be, say, case- or accent-insensitive.  For that, you need to
-   choose an appropriate LC_COLLATE setting
+   choose an appropriate LOCALE setting
and set the collation to not deterministic here.
   
 
-- 
2.44.0

Re: small documentation fixes related to collations/ICU

[Kashif Zeeshan]

Looks good.

On Mon, Apr 29, 2024 at 12:05 PM Peter Eisentraut 
wrote:

> I found two mistakes related to collation and/or ICU support in the
> documentation that should probably be fixed and backpatched.  See
> attached patches.

Use "unique keys" to enhance outer join removal

[Antonin Houska]

While the "unique keys" feature [1] is still under development, I'm thinking
how it could be used to enhance the removal of useless outer joins. Is
something really bad about the 0002 patch attached?

I recognize it may be weird that a join relation possibly produces non-join
paths (e.g. SeqScan), but right now don't have better idea where the new code
should appear. I considered planning the subqueries before the existing call
of remove_useless_joins(), to make the unique keys available earlier. However
it seems that more items can be added to 'baserestrictinfo' of the subquery
relation after that. Thus by planning the subquery too early we could miss
some opportunities to push clauses down to the subquery.

Please note that this patch depends on [1], which enhances
rel_is_distinct_for() and thus makes join_is_removable() a bit smareter. Also
note that 0001 is actually a minor fix to [1].

[1] https://www.postgresql.org/message-id/7971.1713526758%40antos
-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

>From a0be4ee7698ff03d6c22398f20fd2c7efadbff45 Mon Sep 17 00:00:00 2001
From: Antonin Houska 
Date: Mon, 29 Apr 2024 07:53:00 +0200
Subject: [PATCH 1/2] Undo comment changes.

These belong to the following patch.
---
 src/test/regress/expected/join.out | 11 +--
 src/test/regress/sql/join.sql  | 13 ++---
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 571198a86a..fa407d37f5 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5648,12 +5648,11 @@ select d.* from d left join (select distinct * from b) s
  Seq Scan on d
 (1 row)
 
--- when the GROUP BY contains a column that is not in the join condition, join
--- removal takes place too, due to "unique keys" (Note: as of 9.6, we notice
--- that b.id is a primary key and so drop b.c_id from the GROUP BY of the
--- resulting plan; but this happens too late for join removal in the outer
--- plan level.)
-explain (costs off, verbose on)
+-- join removal is not possible when the GROUP BY contains a column that is
+-- not in the join condition.  (Note: as of 9.6, we notice that b.id is a
+-- primary key and so drop b.c_id from the GROUP BY of the resulting plan;
+-- but this happens too late for join removal in the outer plan level.)
+explain (costs off)
 select d.* from d left join (select * from b group by b.id, b.c_id) s
   on d.a = s.id;
QUERY PLAN   
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index bf8fb27072..c4c6c7b8ba 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2047,17 +2047,16 @@ explain (costs off)
 select d.* from d left join (select distinct * from b) s
   on d.a = s.id and d.b = s.c_id;
 
--- when the GROUP BY contains a column that is not in the join condition, join
--- removal takes place too, due to "unique keys" (Note: as of 9.6, we notice
--- that b.id is a primary key and so drop b.c_id from the GROUP BY of the
--- resulting plan; but this happens too late for join removal in the outer
--- plan level.)
-explain (costs off, verbose on)
+-- join removal is not possible when the GROUP BY contains a column that is
+-- not in the join condition.  (Note: as of 9.6, we notice that b.id is a
+-- primary key and so drop b.c_id from the GROUP BY of the resulting plan;
+-- but this happens too late for join removal in the outer plan level.)
+explain (costs off)
 select d.* from d left join (select * from b group by b.id, b.c_id) s
   on d.a = s.id;
 
 -- similarly, but keying off a DISTINCT clause
-explain (costs off, verbose on)
+explain (costs off)
 select d.* from d left join (select distinct * from b) s
   on d.a = s.id;
 
-- 
2.44.0

>From cb0166a3e1f1f5ff88634c38c2474de16825625a Mon Sep 17 00:00:00 2001
From: Antonin Houska 
Date: Mon, 29 Apr 2024 08:24:42 +0200
Subject: [PATCH 2/2] Use Unique Keys to remove an useless left join.

To consider an outer join useless, we need to prove that the inner relation is
unique for given join clause(s). We may be unable do that at early stage of
planning, especially if the inner relation is a subquery.

The new "unique keys" feature (still under development) can resolve this
problem by re-trying the removal at later stage, when the unique keys have
(possibly) been initialized.
---
 src/backend/optimizer/path/joinpath.c | 35 
 src/backend/optimizer/path/joinrels.c | 10 +
 src/backend/optimizer/plan/analyzejoins.c |  4 +-
 src/include/optimizer/paths.h |  5 +++
 src/include/optimizer/planmain.h  |  1 +
 src/test/regress/expected/join.out| 50 +++
 src/test/regress/sql/join.sql | 13 +++---
 7 files changed, 73 insertions(+), 45 deletions(-)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 5be8da9e09..3dc9dc7856 1006

Re: Direct SSL connection with ALPN and HBA rules

[Heikki Linnakangas]

On 26/04/2024 02:23, Jacob Champion wrote:

On Thu, Apr 25, 2024 at 2:50 PM Heikki Linnakangas  wrote:

I think that comes down to the debate upthread, and whether you think
it's a performance tweak or a security feature. My take on it is,
`direct` mode is performance, and `requiredirect` is security.


Agreed, although the the security benefits from `requiredirect` are
pretty vague. It reduces the attack surface, but there are no known
issues with the 'postgres' or 'direct' negotiation either.


I think reduction in attack surface is a concrete security benefit,
not a vague one. True, I don't know of any exploits today, but that
seems almost tautological -- if there were known exploits in our
upgrade handshake, I assume we'd be working to fix them ASAP?


Sure, we'd try to fix them ASAP. But we've had the SSLRequest 
negotiation since time immemorial. If a new vulnerability is found, it's 
unlikely that we'd need to disable the SSLRequest negotiation altogether 
to fix it. We'd be in serious trouble with back-branches in that case. 
There's no sudden need to have a kill-switch for it.


Taking that to the extreme, you could argue for a kill-switch for every 
feature, just in case there's a vulnerability in them. I agree that 
authentication is more sensitive so reducing the surface of that is more 
reasonable. And but nevertheless.


(This discussion is moot though, because we do have the 
sslnegotiation=requiredirect mode, so you can disable the SSLRequest 
negotiation.)



Perhaps 'requiredirect' should be renamed to 'directonly'?


If it's agreed that we don't want to require a stronger sslmode for
that sslnegotiation setting, then that would probably be an
improvement. But who is the target user for
`sslnegotiation=directonly`, in your opinion? Would they ever have a
reason to use a weak sslmode?


It's unlikely, I agree. A user who's sophisticated enough to use 
sslnegotiation=directonly would probably also want sslmode=require and 
require_auth=scram-sha256 and channel_binding=require. Or 
sslmode=verify-full. But in principle they're orthogonal. If you only 
have v17 servers in your environment, so you know all servers support 
direct negotiation if they support SSL at all, but a mix of servers with 
and without SSL, sslnegotiation=directonly would reduce roundtrips with 
sslmode=prefer.


Making requiredirect to imply sslmode=require, or error out unless you 
also set sslmode=require, feels like a cavalier way of forcing SSL. We 
should have a serious discussion on making sslmode=require the default 
instead. That would be a more direct way of nudging people to use SSL. 
It would cause a lot of breakage, but it would also be a big improvement 
to security.


Consider how sslnegotiation=requiredirect/directonly would feel, if we 
made sslmode=require the default. If you explicitly set "sslmode=prefer" 
or "sslmode=disable", it would be annoying if you would also need to 
remove "sslnegotiation=requiredirect" from your connection string.


I'm leaning towards renaming sslnegotiation=requiredirect to 
sslnegotiation=directonly at this point.



I'm not sure about this either. The 'gssencmode' option is already
quite weird in that it seems to override the "require"d priority of
"sslmode=require", which it IMO really shouldn't.


Yeah, that combination is weird. I think we should forbid it. But that's
separate from sslnegotiation.


Separate but related, IMO. If we were all hypothetically okay with
gssencmode ignoring `sslmode=require`, then it's hard for me to claim
that `sslnegotiation=requiredirect` should behave differently. On the
other hand, if we're not okay with that and we'd like to change it,
it's easier for me to argue that `requiredirect` should also be
stricter from the get-go.


I think the best way forward for those is something like a new 
"require_proto" parameter that you suggested upthread. Perhaps call it 
"encryption", with options "none", "ssl", "gss" so that you can provide 
multiple options and libpq will try them in the order specified. For 
example:


encryption=none
encryption=ssl, none  # like sslmode=prefer
encryption=gss
encryption=gss, ssl   # try GSS first, then SSL
encryption=ssl, gss   # try SSL first, then GSS

This would make gssencmode and sslmode=disable/allow/prefer/require 
settings obsolete. sslmode would stil be needed to distinguish between 
verify-ca/verify-full though. But sslnegotiation would be still 
orthogonal to that.


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

Re: A failure in prepared_xacts test

[Richard Guo]

On Mon, Apr 29, 2024 at 1:11 PM Tom Lane  wrote:

> Up to now, we've only worried about whether tests running in parallel
> within a single test suite can interact.  It's quite scary to think
> that the meson setup has expanded the possibility of interactions
> to our entire source tree.  Maybe that was a bad idea and we should
> fix the meson infrastructure to not do that.  I fear that otherwise,
> we'll get bit regularly by very-low-probability bugs of this kind.


I have the same concern.  I suspect that the scan of pg_prepared_xacts
is not the only test that could cause problems when running in parallel
to other tests from the entire source tree.

Thanks
Richard

Re: Synchronizing slots from primary to standby

[shveta malik]

On Mon, Apr 29, 2024 at 11:38 AM shveta malik  wrote:
>
> On Mon, Apr 29, 2024 at 10:57 AM Zhijie Hou (Fujitsu)
>  wrote:
> >
> > On Friday, March 15, 2024 10:45 PM Bertrand Drouvot 
> >  wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Mar 14, 2024 at 02:22:44AM +, Zhijie Hou (Fujitsu) wrote:
> > > > Hi,
> > > >
> > > > Since the standby_slot_names patch has been committed, I am attaching
> > > > the last doc patch for review.
> > > >
> > >
> > > Thanks!
> > >
> > > 1 ===
> > >
> > > +   continue subscribing to publications now on the new primary server
> > > without
> > > +   any data loss.
> > >
> > > I think "without any data loss" should be re-worded in this context. Data 
> > > loss in
> > > the sense "data committed on the primary and not visible on the 
> > > subscriber in
> > > case of failover" can still occurs (in case synchronous replication is 
> > > not used).
> > >
> > > 2 ===
> > >
> > > +   If the result (failover_ready) of both above steps 
> > > is
> > > +   true, existing subscriptions will be able to continue without data 
> > > loss.
> > > +  
> > >
> > > I don't think that's true if synchronous replication is not used. Say,
> > >
> > > - synchronous replication is not used
> > > - primary is not able to reach the standby anymore and standby_slot_names 
> > > is
> > > set
> > > - new data is inserted into the primary
> > > - then not replicated to subscriber (due to standby_slot_names)
> > >
> > > Then I think the both above steps will return true but data would be lost 
> > > in case
> > > of failover.
> >
> > Thanks for the comments, attach the new version patch which reworded the
> > above places.
>
> Thanks for the patch.
>
> Few comments:
>
> 1)  Tested the steps, one of the queries still refers to
> 'conflict_reason'. I think it should refer 'conflicting'.
>
> 2) Will it be good to mention that in case of planned promotion, it is
> recommended to run  pg_sync_replication_slots() as last sync attempt
> before we run failvoer-ready validation steps? This can be mentioned
> in high-availaibility.sgml of current patch

I recall now that with the latest fix, we cannot run
pg_sync_replication_slots() unless we disable the slot-sync worker.
Considering that, I think it will be too many steps just to run the
SQL function at the end without much value added. Thus we can skip
this point, we can rely on slot sync worker completely.

thanks
Shveta

Re: Synchronizing slots from primary to standby

[shveta malik]

On Mon, Apr 29, 2024 at 11:38 AM shveta malik  wrote:
>
> On Mon, Apr 29, 2024 at 10:57 AM Zhijie Hou (Fujitsu)
>  wrote:
> >
> > On Friday, March 15, 2024 10:45 PM Bertrand Drouvot 
> >  wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Mar 14, 2024 at 02:22:44AM +, Zhijie Hou (Fujitsu) wrote:
> > > > Hi,
> > > >
> > > > Since the standby_slot_names patch has been committed, I am attaching
> > > > the last doc patch for review.
> > > >
> > >
> > > Thanks!
> > >
> > > 1 ===
> > >
> > > +   continue subscribing to publications now on the new primary server
> > > without
> > > +   any data loss.
> > >
> > > I think "without any data loss" should be re-worded in this context. Data 
> > > loss in
> > > the sense "data committed on the primary and not visible on the 
> > > subscriber in
> > > case of failover" can still occurs (in case synchronous replication is 
> > > not used).
> > >
> > > 2 ===
> > >
> > > +   If the result (failover_ready) of both above steps 
> > > is
> > > +   true, existing subscriptions will be able to continue without data 
> > > loss.
> > > +  
> > >
> > > I don't think that's true if synchronous replication is not used. Say,
> > >
> > > - synchronous replication is not used
> > > - primary is not able to reach the standby anymore and standby_slot_names 
> > > is
> > > set
> > > - new data is inserted into the primary
> > > - then not replicated to subscriber (due to standby_slot_names)
> > >
> > > Then I think the both above steps will return true but data would be lost 
> > > in case
> > > of failover.
> >
> > Thanks for the comments, attach the new version patch which reworded the
> > above places.
>
> Thanks for the patch.
>
> Few comments:
>
> 1)  Tested the steps, one of the queries still refers to
> 'conflict_reason'. I think it should refer 'conflicting'.
>
> 2) Will it be good to mention that in case of planned promotion, it is
> recommended to run  pg_sync_replication_slots() as last sync attempt
> before we run failvoer-ready validation steps? This can be mentioned
> in high-availaibility.sgml of current patch

I recall now that with the latest fix, we cannot run
pg_sync_replication_slots() unless we disable the slot-sync worker.
Considering that, I think it will be too many steps just to run the
SQL function at the end without much value added. Thus we can skip
this point, we can rely on slot sync worker completely.

thanks
Shveta

Re: A failure in prepared_xacts test

[Richard Guo]

On Mon, Apr 29, 2024 at 2:58 PM Michael Paquier  wrote:

> On Mon, Apr 29, 2024 at 01:32:40AM -0400, Tom Lane wrote:
> > (BTW, on the same logic, should ecpg's twophase.pgc be using a
> > prepared-transaction name that's less generic than "gxid"?)
>
> I've hesitated a few seconds about that before sending my patch, but
> refrained because this stuff does not care about the contents of
> pg_prepared_xacts.  I'd be OK to use something like an "ecpg_regress"
> or something similar there.


I noticed that some TAP tests from recovery and subscription would
select the count from pg_prepared_xacts.  I wonder if these tests would
be affected if there are any prepared transactions on the backend.

Thanks
Richard

Re: Support a wildcard in backtrace_functions

[Peter Eisentraut]

On 27.04.24 00:16, Michael Paquier wrote:

On Fri, Apr 26, 2024 at 02:39:16PM -0400, Tom Lane wrote:

Well, in that case we have to have some kind of control GUC, and
I think the consensus is that the one we have now is under-designed.
So I also vote for a full revert and try again in v18.


Okay, fine by me to move on with a revert.


Ok, it's reverted.

Re: A failure in prepared_xacts test

[Michael Paquier]

On Mon, Apr 29, 2024 at 05:11:19PM +0800, Richard Guo wrote:
> I noticed that some TAP tests from recovery and subscription would
> select the count from pg_prepared_xacts.  I wonder if these tests would
> be affected if there are any prepared transactions on the backend.

TAP tests run in isolation of the rest with their own clusters
initialized from a copy initdb'd (rather than initdb because that's
much cheaper), so these scans are OK left alone.
--
Michael


signature.asc
Description: PGP signature

Re: Direct SSL connection with ALPN and HBA rules

[Heikki Linnakangas]

On 23/04/2024 10:07, Michael Paquier wrote:

In the documentation of PQsslAttribute(), it is mentioned that empty
string is returned for "alpn" if ALPN was not used, however the code
returns NULL in this case:
 SSL_get0_alpn_selected(conn->ssl, &data, &len);
 if (data == NULL || len == 0 || len > sizeof(alpn_str) - 1)
 return NULL;


Good catch. I changed the code to return an empty string, as the 
documentation says.


I considered if NULL or empty string would be better here. The docs for 
PQsslAttribute also says:


"Returns NULL if the connection does not use SSL or the specified 
attribute name is not defined for the library in use."


If a caller wants to distinguish between "libpq or the SSL library 
doesn't support ALPN at all" from "the server didn't support ALPN", you 
can tell from whether PQsslAttribute returns NULL or an empty string. So 
I think an empty string is better.


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

Re: speed up a logical replica setup

[Amit Kapila]

On Wed, Mar 27, 2024 at 1:47 AM Euler Taveira  wrote:
>
> On Tue, Mar 26, 2024, at 4:12 PM, Tomas Vondra wrote:
>
> Perhaps I'm missing something, but why is NUM_CONN_ATTEMPTS even needed?
> Why isn't recovery_timeout enough to decide if wait_for_end_recovery()
> waited long enough?
>
>
> It was an attempt to decoupled a connection failure (that keeps streaming the
> WAL) from recovery timeout. The NUM_CONN_ATTEMPTS guarantees that if the 
> primary
> is gone during the standby recovery process, there is a way to bail out.
>

I think we don't need to check primary if the WAL corresponding to
consistent_lsn is already present on the standby. Shouldn't we first
check that? Once we ensure that the required WAL is copied, just
checking server_is_in_recovery() should be sufficient. I feel that
will be a direct way of ensuring what is required rather than
indirectly verifying the same (by checking pg_stat_wal_receiver) as we
are doing currently.

-- 
With Regards,
Amit Kapila.

Join removal and attr_needed cleanup

[Antonin Houska]

The attached patch tries to fix a corner case where attr_needed of an inner
relation of an OJ contains the join relid only because another,
already-removed OJ, needed some of its attributes. The unnecessary presence of
the join relid in attr_needed can prevent the planner from further join
removals.

Do cases like this seem worth the effort and is the logic I use correct?

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

>From 279856bf97ce08c0c2e0c736a00831bf6324713b Mon Sep 17 00:00:00 2001
From: Antonin Houska 
Date: Mon, 29 Apr 2024 11:34:30 +0200
Subject: [PATCH] Cleanup attr_needed properly after join removal.

If an outer join (J1) was removed and if the remaining outer relation is also
an outer join (J2), the inner relation of J2 may still have the J2's relid in
the attr_needed set, because its attribute was referenced by J1. However,
after removal of J1, it's possible that no other join (nor the query
targetlist) actually needs any attribute of J2, and so the J2's relid should
not be in attr_needed anymore.

This patch tries to spend more effort on checking the contents of attr_needed
after removal of J1 so that J2 can potentially be removed as well.
---
 src/backend/optimizer/plan/analyzejoins.c | 117 +-
 src/test/regress/expected/join.out|   9 ++
 src/test/regress/sql/join.sql |   5 +
 3 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 506fccd20c..1b9623efaa 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -46,6 +46,10 @@ bool		enable_self_join_removal;
 /* local functions */
 static bool join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo);
 
+static bool innerrel_attrs_needed_above_outer_join(PlannerInfo *root,
+   RelOptInfo *innerrel,
+   SpecialJoinInfo *sjinfo,
+   Relids joinrelids);
 static void remove_leftjoinrel_from_query(PlannerInfo *root, int relid,
 		  SpecialJoinInfo *sjinfo);
 static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
@@ -183,6 +187,8 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 	List	   *clause_list = NIL;
 	ListCell   *l;
 	int			attroff;
+	Oid			remove_ojrelid;
+	bool		remove_ojrelid_valid;
 
 	/*
 	 * Must be a left join to a single baserel, else we aren't going to be
@@ -218,6 +224,13 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 	joinrelids = bms_copy(inputrelids);
 	joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
 
+	/*
+	 * By default there's no reason to remove sjinfo->ojrelid from
+	 * attr_needed, see below.
+	 */
+	remove_ojrelid = InvalidOid;
+	remove_ojrelid_valid = false;
+
 	/*
 	 * We can't remove the join if any inner-rel attributes are used above the
 	 * join.  Here, "above" the join includes pushed-down conditions, so we
@@ -233,7 +246,38 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 		 attroff >= 0;
 		 attroff--)
 	{
-		if (!bms_is_subset(innerrel->attr_needed[attroff], inputrelids))
+		Relids	attr_needed = innerrel->attr_needed[attroff];
+
+		/*
+		 * If this join was among outer relations of an already-removed join,
+		 * attr_needed may still contain the relid of the current join because
+		 * join clauses of the removed join referenced it. Re-check if another
+		 * join can still reference this join and if not, remove it from
+		 * attr_needed.
+		 */
+		if (bms_is_member(sjinfo->ojrelid, attr_needed))
+		{
+			/* Do the following check only once per inner relation. */
+			if (!remove_ojrelid_valid)
+			{
+/*
+ * If no clause requires the join relid anymore, remember that
+ * we should remove it from attr_needed.
+ */
+if (!innerrel_attrs_needed_above_outer_join(root, innerrel,
+			sjinfo,
+			joinrelids))
+	remove_ojrelid = sjinfo->ojrelid;
+
+remove_ojrelid_valid = true;
+			}
+
+			if (OidIsValid(remove_ojrelid))
+innerrel->attr_needed[attroff] = bms_del_member(attr_needed,
+remove_ojrelid);
+		}
+
+		if (!bms_is_subset(attr_needed, inputrelids))
 			return false;
 	}
 
@@ -333,6 +377,77 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
 	return false;
 }
 
+/*
+ * innerrel_attrs_needed_above_outer_join
+ *	  Re-check whether attributes of inner relation of an OJ are still needed
+ *	  above that OJ.
+ */
+static bool
+innerrel_attrs_needed_above_outer_join(PlannerInfo *root, RelOptInfo *innerrel,
+	   SpecialJoinInfo *sjinfo,
+	   Relids joinrelids)
+{
+	bool	found = false;
+	ListCell	*l;
+
+	/* Check the join clauses */
+	foreach(l, innerrel->joininfo)
+	{
+		RestrictInfo *ri = lfirst_node(RestrictInfo, l);
+
+		if (bms_is_member(sjinfo->ojrelid, ri->required_relids))
+			return true;
+	}
+
+	/*
+	 * Also check if any clause generated from EC may need the ojrelid.
+	 */
+	foreach(l, root->eq_clas

Re: speed up a logical replica setup

[Amit Kapila]

On Tue, Mar 26, 2024 at 8:24 AM Euler Taveira  wrote:
>
> On Mon, Mar 25, 2024, at 1:06 PM, Hayato Kuroda (Fujitsu) wrote:
>
> The first patch implements a combination of (1) and (2).
>
> ## Analysis for failure 2
>
> According to [2], the physical replication slot which is specified as 
> primary_slot_name
> was not used by the walsender process. At that time walsender has not existed.
>
> ```
> ...
> pg_createsubscriber: publisher: current wal senders: 0
> pg_createsubscriber: command is: SELECT 1 FROM 
> pg_catalog.pg_replication_slots WHERE active AND slot_name = 'physical_slot'
> pg_createsubscriber: error: could not obtain replication slot information: 
> got 0 rows, expected 1 row
> ...
> ```
>
> Currently standby must be stopped before the command and current code does not
> block the flow to ensure the replication is started. So there is a possibility
> that the checking is run before walsender is launched.
>
> One possible approach is to wait until the replication starts. Alternative 
> one is
> to ease the condition.
>
>
> That's my suggestion too. I reused NUM_CONN_ATTEMPTS (that was renamed to
> NUM_ATTEMPTS in the first patch). See second patch.
>

How can we guarantee that the slot can become active after these many
attempts? It still could be possible that on some slow machines it
didn't get activated even after NUM_ATTEMPTS. BTW, in the first place,
why do we need to ensure that the 'primary_slot_name' is active on the
primary?

-- 
With Regards,
Amit Kapila.

Re: speed up a logical replica setup

[Euler Taveira]

On Mon, Apr 29, 2024, at 6:56 AM, Amit Kapila wrote:
> On Wed, Mar 27, 2024 at 1:47 AM Euler Taveira  wrote:
> >
> > On Tue, Mar 26, 2024, at 4:12 PM, Tomas Vondra wrote:
> >
> > Perhaps I'm missing something, but why is NUM_CONN_ATTEMPTS even needed?
> > Why isn't recovery_timeout enough to decide if wait_for_end_recovery()
> > waited long enough?
> >
> >
> > It was an attempt to decoupled a connection failure (that keeps streaming 
> > the
> > WAL) from recovery timeout. The NUM_CONN_ATTEMPTS guarantees that if the 
> > primary
> > is gone during the standby recovery process, there is a way to bail out.
> >
> 
> I think we don't need to check primary if the WAL corresponding to
> consistent_lsn is already present on the standby. Shouldn't we first
> check that? Once we ensure that the required WAL is copied, just
> checking server_is_in_recovery() should be sufficient. I feel that
> will be a direct way of ensuring what is required rather than
> indirectly verifying the same (by checking pg_stat_wal_receiver) as we
> are doing currently.

How would you check it? WAL file? During recovery, you are not allowed to use
pg_current_wal_lsn.

Tomas suggested to me off-list that we should adopt a simple solution in
wait_for_end_recovery: wait for recovery_timeout without additional checks
(which means remove the pg_stat_wal_receiver logic).  When we have additional
information that we can reliably use in this function, we can add it. Hence, it
is also easy to adjust the PG_TEST_TIMEOUT_DEFAULT to have stable tests.


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

RE: Synchronizing slots from primary to standby

[Zhijie Hou (Fujitsu)]

On Monday, April 29, 2024 5:11 PM shveta malik  wrote:
> 
> On Mon, Apr 29, 2024 at 11:38 AM shveta malik 
> wrote:
> >
> > On Mon, Apr 29, 2024 at 10:57 AM Zhijie Hou (Fujitsu)
> >  wrote:
> > >
> > > On Friday, March 15, 2024 10:45 PM Bertrand Drouvot
>  wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Thu, Mar 14, 2024 at 02:22:44AM +, Zhijie Hou (Fujitsu) wrote:
> > > > > Hi,
> > > > >
> > > > > Since the standby_slot_names patch has been committed, I am
> > > > > attaching the last doc patch for review.
> > > > >
> > > >
> > > > Thanks!
> > > >
> > > > 1 ===
> > > >
> > > > +   continue subscribing to publications now on the new primary
> > > > + server
> > > > without
> > > > +   any data loss.
> > > >
> > > > I think "without any data loss" should be re-worded in this
> > > > context. Data loss in the sense "data committed on the primary and
> > > > not visible on the subscriber in case of failover" can still occurs (in 
> > > > case
> synchronous replication is not used).
> > > >
> > > > 2 ===
> > > >
> > > > +   If the result (failover_ready) of both above 
> > > > steps is
> > > > +   true, existing subscriptions will be able to continue without data
> loss.
> > > > +  
> > > >
> > > > I don't think that's true if synchronous replication is not used.
> > > > Say,
> > > >
> > > > - synchronous replication is not used
> > > > - primary is not able to reach the standby anymore and
> > > > standby_slot_names is set
> > > > - new data is inserted into the primary
> > > > - then not replicated to subscriber (due to standby_slot_names)
> > > >
> > > > Then I think the both above steps will return true but data would
> > > > be lost in case of failover.
> > >
> > > Thanks for the comments, attach the new version patch which reworded
> > > the above places.
> >
> > Thanks for the patch.
> >
> > Few comments:
> >
> > 1)  Tested the steps, one of the queries still refers to
> > 'conflict_reason'. I think it should refer 'conflicting'.

Thanks for catching this. Fixed.

> >
> > 2) Will it be good to mention that in case of planned promotion, it is
> > recommended to run  pg_sync_replication_slots() as last sync attempt
> > before we run failvoer-ready validation steps? This can be mentioned
> > in high-availaibility.sgml of current patch
> 
> I recall now that with the latest fix, we cannot run
> pg_sync_replication_slots() unless we disable the slot-sync worker.
> Considering that, I think it will be too many steps just to run the SQL 
> function at
> the end without much value added. Thus we can skip this point, we can rely on
> slot sync worker completely.

Agreed. I didn't change this.

Here is the V3 doc patch.

Best Regards,
Hou zj


v3-0001-Document-the-steps-to-check-if-the-standby-is-rea.patch
Description:  v3-0001-Document-the-steps-to-check-if-the-standby-is-rea.patch

Re: speed up a logical replica setup

[Amit Kapila]

On Mon, Apr 29, 2024 at 5:23 PM Euler Taveira  wrote:
>
> On Mon, Apr 29, 2024, at 6:56 AM, Amit Kapila wrote:
>
> On Wed, Mar 27, 2024 at 1:47 AM Euler Taveira  wrote:
> >
> > On Tue, Mar 26, 2024, at 4:12 PM, Tomas Vondra wrote:
> >
> > Perhaps I'm missing something, but why is NUM_CONN_ATTEMPTS even needed?
> > Why isn't recovery_timeout enough to decide if wait_for_end_recovery()
> > waited long enough?
> >
> >
> > It was an attempt to decoupled a connection failure (that keeps streaming 
> > the
> > WAL) from recovery timeout. The NUM_CONN_ATTEMPTS guarantees that if the 
> > primary
> > is gone during the standby recovery process, there is a way to bail out.
> >
>
> I think we don't need to check primary if the WAL corresponding to
> consistent_lsn is already present on the standby. Shouldn't we first
> check that? Once we ensure that the required WAL is copied, just
> checking server_is_in_recovery() should be sufficient. I feel that
> will be a direct way of ensuring what is required rather than
> indirectly verifying the same (by checking pg_stat_wal_receiver) as we
> are doing currently.
>
>
> How would you check it? WAL file? During recovery, you are not allowed to use
> pg_current_wal_lsn.
>

How about pg_last_wal_receive_lsn()?

-- 
With Regards,
Amit Kapila.

Re: Direct SSL connection with ALPN and HBA rules

[Michael Paquier]

On Mon, Apr 29, 2024 at 12:43:18PM +0300, Heikki Linnakangas wrote:
> If a caller wants to distinguish between "libpq or the SSL library doesn't
> support ALPN at all" from "the server didn't support ALPN", you can tell
> from whether PQsslAttribute returns NULL or an empty string. So I think an
> empty string is better.

Thanks.  I would also have used an empty string to differenciate these
two cases.
--
Michael


signature.asc
Description: PGP signature

Re: Support a wildcard in backtrace_functions

[Robert Haas]

On Mon, Apr 29, 2024 at 5:12 AM Peter Eisentraut  wrote:
> On 27.04.24 00:16, Michael Paquier wrote:
> > On Fri, Apr 26, 2024 at 02:39:16PM -0400, Tom Lane wrote:
> >> Well, in that case we have to have some kind of control GUC, and
> >> I think the consensus is that the one we have now is under-designed.
> >> So I also vote for a full revert and try again in v18.
> >
> > Okay, fine by me to move on with a revert.
>
> Ok, it's reverted.

Thanks, and sorry. :-(

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Tarball builds in the new world order

[Peter Eisentraut]

On 26.04.24 21:24, Tom Lane wrote:

Concretely, I'm proposing the attached.  Peter didn't like
PG_COMMIT_HASH, so I have PG_COMMIT_REFSPEC below, but I'm not
wedded to that if a better name is proposed.


This seems ok to me, but note that we do have an equivalent 
implementation in meson.  If we don't want to update that in a similar 
way, maybe we should disable it.

Re: Direct SSL connection with ALPN and HBA rules

[Robert Haas]

On Mon, Apr 29, 2024 at 4:38 AM Heikki Linnakangas  wrote:
> Making requiredirect to imply sslmode=require, or error out unless you
> also set sslmode=require, feels like a cavalier way of forcing SSL. We
> should have a serious discussion on making sslmode=require the default
> instead. That would be a more direct way of nudging people to use SSL.
> It would cause a lot of breakage, but it would also be a big improvement
> to security.
>
> Consider how sslnegotiation=requiredirect/directonly would feel, if we
> made sslmode=require the default. If you explicitly set "sslmode=prefer"
> or "sslmode=disable", it would be annoying if you would also need to
> remove "sslnegotiation=requiredirect" from your connection string.

I think making sslmode=require the default is pretty unworkable,
unless we also had a way of automatically setting up SSL as part of
initdb or something. Otherwise, we'd have to add sslmode=disable to a
million places just to get the regression tests to work, and every
test cluster anyone spins up locally would break in annoying ways,
too. I had been thinking we might want to change the default to
sslmode=disable and remove allow and prefer, but maybe automating a
basic SSL setup is better. Either way, we should move toward a world
where you either ask for SSL and get it, or don't ask for it and don't
get it. Being halfway in between is bad.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: using extended statistics to improve join estimates

[Justin Pryzby]

On Sun, Apr 28, 2024 at 10:07:01AM +0800, Andy Fan wrote:
> 's/estimiatedcluases/estimatedclauses/' typo error in the
> commit message is not fixed since I have to regenerate all the commits

Maybe you know this, but some of these patches need to be squashed.
Regenerating the patches to address feedback is the usual process.
When they're not squished, it makes it hard to review the content of the
patches.

For example:
[PATCH v1 18/22] Fix error "unexpected system attribute" when join with system 
attr
..adds .sql regression tests, but the expected .out isn't updated until
[PATCH v1 19/22] Fix the incorrect comment on extended stats.

That fixes an elog() in Tomas' original commit, so it should probably be
002 or 003.  It might make sense to keep the first commit separate for
now, since it's nice to keep Tomas' original patch "pristine" to make
more apparent the changes you're proposing.

Another:
[PATCH v1 20/22] Add fastpath when combine the 2 MCV like eqjoinsel_inner.
..doesn't compile without
[PATCH v1 21/22] When mcv->ndimensions == list_length(clauses), handle it same 
as

Your 022 patch fixes a typo in your 002 patch, which means that first
one reads a patch with a typo, and then later, a 10 line long patch
reflowing the comment with a typo fixed.

A good guideline is that each patch should be self-contained, compiling
and passing tests.  Which is more difficult with a long stack of
patches.

-- 
Justin

Re: Tarball builds in the new world order

[Peter Eisentraut]

On 26.04.24 21:24, Tom Lane wrote:

Concretely, I'm proposing the attached.  Peter didn't like
PG_COMMIT_HASH, so I have PG_COMMIT_REFSPEC below, but I'm not
wedded to that if a better name is proposed.


Um, "refspec" leads me here 
, which seems 
like the wrong concept.  I think the more correct concept is "revision" 
(https://git-scm.com/docs/gitrevisions), so something like PG_GIT_REVISION?

Re: Internal error codes triggered by tests

[Justin Pryzby]

I sent a list of user-facing elogs here, a few times.
ZDclRM/jate66...@telsasoft.com

And if someone had expressed an interest, I might have sent a longer
list.

Re: tablecmds.c/MergeAttributes() cleanup

[Robert Haas]

On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat
 wrote:
> Yes please. Probably this issue surfaced again after we reverted compression 
> and storage fix? Please  If that's the case, please add it to the open items.

This is still on the open items list and I'm not clear who, if anyone,
is working on fixing it.

It would be good if someone fixed it. :-)

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Possible data race on Windows (src/bin/pg_dump/parallel.c)

[Ranier Vilela]

Hi,

Per Coverity.

CID 1542943: (#1 of 1): Data race condition (MISSING_LOCK)
3. missing_lock: Accessing slot->AH without holding lock signal_info_lock.
Elsewhere, ParallelSlot.AH is written to with signal_info_lock held 1 out
of 1 times (1 of these accesses strongly imply that it is necessary).

The function DisconnectDatabase effectively writes the ParallelSlot.AH.
So the call in the function archive_close_connection:

if (slot->AH)
DisconnectDatabase(&(slot->AH->public));

It should also be protected on Windows, correct?

Patch attached.

best regards,
Ranier Vilela


fix-possible-data-race-windows-parallel.patch
Description: Binary data

Re: New committers: Melanie Plageman, Richard Guo

[Jimmy Yih]

Big congrats to you two!!!

- Jimmy

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: A failure in prepared_xacts test

[Tom Lane]

Richard Guo  writes:
> I noticed that some TAP tests from recovery and subscription would
> select the count from pg_prepared_xacts.  I wonder if these tests would
> be affected if there are any prepared transactions on the backend.

TAP tests shouldn't be at risk, because there is no "make
installcheck" equivalent for them.  Each TAP test creates its own
database instance (or maybe several), so that instance won't have
anything else going on.

regards, tom lane

Direct SSL connection and ALPN loose ends

[Heikki Linnakangas]

There's been a bunch of bugs, and discussion on the intended behavior of 
sslnegotiation and ALPN. This email summarizes the current status:


## Status and loose ends for beta1

All reported bugs have now been fixed. We now enforce ALPN in all the 
right places. Please let me know if I missed something.


There are two open items remaining that I intend to address in the next 
few days, before beta1:


- I am going to rename sslnegotiation=requiredirect to 
sslnegotiation=directonly. I acknowledge that there is still some debate 
on this: Jacob (and Robert?) would prefer to change the behavior 
instead, so that sslnegotiation=requiredirect would also imply or 
require sslmode=require, while IMHO the settings should be orthogonal so 
that sslmode controls whether SSL is used or not, and sslnegotiation 
controls how the SSL layer is negotiated when SSL is used. Given that 
they are orthogonal, "directonly" is a better name. I will also take 
another look at the documentation, if it needs clarification on that 
point. If you have more comments on whether this is a good idea or not 
or how sslnegotiation should work, please reply on the other thread, 
let's keep this one focused on the overall status. [1]


- The registration of the ALPN name with IANA hasn't been finished yet 
[2]. I originally requested the name "pgsql", but after Peter's comment, 
I changed the request to "postgresql". The string we have in 'master' is 
currently "TBD-pgsql". I'm very confident that the registration will go 
through with "postgresql", so my plan is to commit that change before 
beta1, even if the IANA process hasn't completed by then.


## V18 material

- Add an option to disable traditional SSL negotiation in the server. 
There was discussion on doing this via HBA rules or as a global option, 
and the consensus seems to be for a global option. This would be just to 
reduce the attach surface, there is no known vulnerabilities or other 
issues with the traditional negotiation. And maybe to help with testing. [3]


These are not directly related to sslnegotiation, but came up in the 
discussion:


- Clarify the situation with sslmode=require and gssencmode=require 
combination, by replacing sslmode and gssencmode options with a single 
"encryption=[ssl|gss|none], [...]" option. [4]


- Make sslmode=require the default. This is orthogonal to the SSL 
negotiation, but I think the root cause for the disagreements on 
sslnegotiation is actually that we'd like SSL to be the default. [5]


The details of these need to be hashed out, in particular the 
backwards-compatibility and migration aspects, but the consensus seems 
to be that it's the right direction.


## V19 and beyond

In the future, once v17 is ubiquitous and the ecosystem (pgbouncer etc) 
have added direct SSL support, we can change the default sslnegotiation 
from 'postgres' to 'direct'. I'm thinking 3-5 years from now. In the 
more distant future, we could remove the traditional SSLRequest 
negotiation altogether and always use direct SSL negotiation.


There's no rush on these.

## Retrospective

There were a lot more cleanups required for this work than I expected, 
given that there were little changes to the patches between January and 
March commitfests. I was mostly worried about the refactoring of the 
retry logic in libpq (and about the pre-existing logic too to be honest, 
it was complicated before these changes already). That's why I added a 
lot more tests for that. However, I did not foresee all the ALPN related 
issues. In hindsight, it would have been good to commit most of the ALPN 
changes first, and with more tests. Jacob wrote a python test suite; I 
should've played more with that, that could have demonstrated the ALPN 
issues earlier.


[1] 
https://www.postgresql.org/message-id/CA%2BTgmobV9JEk4AFy61Xw%2B2%2BcCTBqdTsDopkeB%2Bgb81kq3f-o6A%40mail.gmail.com


[2] 
https://mailarchive.ietf.org/arch/msg/tls-reg-review/9LWPzQfOpbc8dTT7vc9ahNeNaiw/


[3] 
https://www.postgresql.org/message-id/CA%2BTgmoaLpDVY2ywqQUfxvKEQZ%2Bnwkabcw_f%3Di4Zyivt9CLjcmA%40mail.gmail.com


[4] 
https://www.postgresql.org/message-id/3a6f126c-e1aa-4dcc-9252-9868308f6cf0%40iki.fi


[5] 
https://www.postgresql.org/message-id/CA%2BTgmoaNkRerEmB9JPgW0FhcJAe337AA%3D5kp6je9KekQhhRbmA%40mail.gmail.com


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

Re: Use streaming read API in ANALYZE

[Nazir Bilal Yavuz]

Hi,

On Mon, 8 Apr 2024 at 04:21, Thomas Munro  wrote:
>
> Pushed.  Thanks Bilal and reviewers!

I wanted to discuss what will happen to this patch now that
27bc1772fc8 is reverted. I am continuing this thread but I can create
another thread if you prefer so.

After the revert of 27bc1772fc8, acquire_sample_rows() became table-AM
agnostic again. So, read stream changes might have to be pushed down
now but there are a couple of roadblocks like Melanie mentioned [1]
before.

Quote from Melanie [1]:

On Thu, 11 Apr 2024 at 19:19, Melanie Plageman
 wrote:
>
> I am working on pushing streaming ANALYZE into heap AM code, and I ran
> into a few roadblocks.
>
> If we want ANALYZE to make the ReadStream object in heap_beginscan()
> (like the read stream implementation of heap sequential and TID range
> scans do), I don't see any way around changing the scan_begin table AM
> callback to take a BufferAccessStrategy at the least (and perhaps also
> the BlockSamplerData).
>
> read_stream_begin_relation() doesn't just save the
> BufferAccessStrategy in the ReadStream, it uses it to set various
> other things in the ReadStream object. callback_private_data (which in
> ANALYZE's case is the BlockSamplerData) is simply saved in the
> ReadStream, so it could be set later, but that doesn't sound very
> clean to me.
>
> As such, it seems like a cleaner alternative would be to add a table
> AM callback for creating a read stream object that takes the
> parameters of read_stream_begin_relation(). But, perhaps it is a bit
> late for such additions.

If we do not want to add a new table AM callback like Melanie
mentioned, it is pretty much required to pass BufferAccessStrategy and
BlockSamplerData to the initscan().

> It also opens us up to the question of whether or not sequential scan
> should use such a callback instead of making the read stream object in
> heap_beginscan().
>
> I am happy to write a patch that does any of the above. But, I want to
> raise these questions, because perhaps I am simply missing an obvious
> alternative solution.

I wonder the same, I could not think of any alternative solution to
this problem.

Another quote from Melanie [2] in the same thread:

On Thu, 11 Apr 2024 at 20:48, Melanie Plageman
 wrote:
>
> I will also say that, had this been 6 months ago, I would probably
> suggest we restructure ANALYZE's table AM interface to accommodate
> read stream setup and to address a few other things I find odd about
> the current code. For example, I think creating a scan descriptor for
> the analyze scan in acquire_sample_rows() is quite odd. It seems like
> it would be better done in the relation_analyze callback. The
> relation_analyze callback saves some state like the callbacks for
> acquire_sample_rows() and the Buffer Access Strategy. But at least in
> the heap implementation, it just saves them in static variables in
> analyze.c. It seems like it would be better to save them in a useful
> data structure that could be accessed later. We have access to pretty
> much everything we need at that point (in the relation_analyze
> callback). I also think heap's implementation of
> table_beginscan_analyze() doesn't need most of
> heap_beginscan()/initscan(), so doing this instead of something
> ANALYZE specific seems more confusing than helpful.

If we want to implement ANALYZE specific counterparts of
heap_beginscan()/initscan(); we may think of passing
BufferAccessStrategy and BlockSamplerData to them.

Also, there is an ongoing(?) discussion about a few problems /
improvements about the acquire_sample_rows() mentioned at the end of
the 'Table AM Interface Enhancements' thread [3]. Should we wait for
these discussions to be resolved or can we resume working on this
patch?

Any kind of feedback would be appreciated.

[1] 
https://www.postgresql.org/message-id/CAAKRu_ZxU6hucckrT1SOJxKfyN7q-K4KU1y62GhDwLBZWG%2BROg%40mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAAKRu_YkphAPNbBR2jcLqnxGhDEWTKhYfLFY%3D0R_oG5LHBH7Gw%40mail.gmail.com
[3] 
https://www.postgresql.org/message-id/flat/CAPpHfdurb9ycV8udYqM%3Do0sPS66PJ4RCBM1g-bBpvzUfogY0EA%40mail.gmail.com

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Tarball builds in the new world order

[Tom Lane]

Peter Eisentraut  writes:
> On 26.04.24 21:24, Tom Lane wrote:
>> Concretely, I'm proposing the attached.  Peter didn't like
>> PG_COMMIT_HASH, so I have PG_COMMIT_REFSPEC below, but I'm not
>> wedded to that if a better name is proposed.

> This seems ok to me, but note that we do have an equivalent 
> implementation in meson.  If we don't want to update that in a similar 
> way, maybe we should disable it.

OK.  After poking at that for awhile, it seemed like "default to
HEAD" fits into meson a lot better than "throw an error if the
variable isn't set", so I switched to doing it like that.
One reason is that AFAICT you can only set the variable during
"meson setup" not during "ninja".  This won't matter to the
tarball build script, which does a one-off configuration run
anyway.  But for manual use, a movable target like HEAD might be
more convenient given that behavior.

I tested this by building tarballs using the makefiles on a RHEL8
box, and using meson on my MacBook (with recent MacPorts tools).
I got bit-for-bit identical files, which I found rather impressive
given the gap between the platforms.  Maybe this "reproducible builds"
wheeze will actually work.

I also changed the variable name to PG_GIT_REVISION per your
other suggestion.

regards, tom lane

diff --git a/GNUmakefile.in b/GNUmakefile.in
index 30553b2a95..9e41794f60 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -87,6 +87,9 @@ update-unicode: | submake-generated-headers submake-libpgport
 distdir	= postgresql-$(VERSION)
 dummy	= =install=
 
+# git revision to be packaged
+PG_GIT_REVISION ?= HEAD
+
 GIT = git
 
 dist: $(distdir).tar.gz $(distdir).tar.bz2
@@ -102,10 +105,10 @@ distdir-location:
 # on, Unix machines.
 
 $(distdir).tar.gz:
-	$(GIT) -C $(srcdir) -c core.autocrlf=false archive --format tar.gz -9 --prefix $(distdir)/ HEAD -o $(abs_top_builddir)/$@
+	$(GIT) -C $(srcdir) -c core.autocrlf=false archive --format tar.gz -9 --prefix $(distdir)/ $(PG_GIT_REVISION) -o $(abs_top_builddir)/$@
 
 $(distdir).tar.bz2:
-	$(GIT) -C $(srcdir) -c core.autocrlf=false -c tar.tar.bz2.command='$(BZIP2) -c' archive --format tar.bz2 --prefix $(distdir)/ HEAD -o $(abs_top_builddir)/$@
+	$(GIT) -C $(srcdir) -c core.autocrlf=false -c tar.tar.bz2.command='$(BZIP2) -c' archive --format tar.bz2 --prefix $(distdir)/ $(PG_GIT_REVISION) -o $(abs_top_builddir)/$@
 
 distcheck: dist
 	rm -rf $(dummy)
diff --git a/meson.build b/meson.build
index cdfd31377d..1c0579d5a6 100644
--- a/meson.build
+++ b/meson.build
@@ -3469,6 +3469,8 @@ bzip2 = find_program('bzip2', required: false, native: true)
 
 distdir = meson.project_name() + '-' + meson.project_version()
 
+pg_git_revision = get_option('PG_GIT_REVISION')
+
 # Note: core.autocrlf=false is needed to avoid line-ending conversion
 # in case the environment has a different setting.  Without this, a
 # tarball created on Windows might be different than on, and unusable
@@ -3483,7 +3485,7 @@ tar_gz = custom_target('tar.gz',
 '-9',
 '--prefix', distdir + '/',
 '-o', join_paths(meson.build_root(), '@OUTPUT@'),
-'HEAD', '.'],
+pg_git_revision],
   output: distdir + '.tar.gz',
 )
 
@@ -3497,7 +3499,7 @@ if bzip2.found()
   '--format', 'tar.bz2',
   '--prefix', distdir + '/',
   '-o', join_paths(meson.build_root(), '@OUTPUT@'),
-  'HEAD', '.'],
+  pg_git_revision],
 output: distdir + '.tar.bz2',
   )
 else
diff --git a/meson_options.txt b/meson_options.txt
index 249ecc5ffd..246cecf382 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -55,6 +55,9 @@ option('atomics', type: 'boolean', value: true,
 option('spinlocks', type: 'boolean', value: true,
   description: 'Use spinlocks')
 
+option('PG_GIT_REVISION', type: 'string', value: 'HEAD',
+  description: 'git revision to be packaged by pgdist target')
+
 
 # Compilation options
 
--- mk-one-release.orig	2024-04-23 17:30:08.983226671 -0400
+++ mk-one-release	2024-04-29 12:10:38.106723387 -0400
@@ -39,13 +39,17 @@ mkdir pgsql
 git archive ${gitref} | tar xf - -C pgsql
 
 # Include the git ref in the output tarballs
+# (This has no effect with v17 and up; instead we rely on "git archive"
+# to include the commit hash in the tar header)
 echo ${gitref} >pgsql/.gitrevision
 
 cd pgsql
 ./configure
 
 # Produce .tar.gz and .tar.bz2 files
-make dist
+# (With v17 and up, this will repeat the "git archive" call; the contents
+# of the working directory don't matter directly to the results.)
+make dist PG_GIT_REVISION=${gitref}
 
 # Generate md5 and sha256 sums, and copy files to output
 for x in *.tar.*; do

Possible to get LIMIT in an index access method?

[Chris Cleveland]

I'm developing an index access method.

SELECT *
FROM foo
WHERE col <=> constant
ORDER BY col <==> constant
LIMIT 10;

I'm successfully getting the WHERE and the ORDER BY clauses in my
beginscan() method. Is there any way to get the LIMIT (or OFFSET, for that
matter)?

My access method is designed such that you have to fetch the entire result
set in one go. It's not streaming, like most access methods. As such, it
would be very helpful to know up front how many items I need to fetch from
the index.

-- 
Chris Cleveland
312-339-2677 mobile

re: Direct SSL connection and ALPN loose ends

[Ranier Vilela]

Hi,

With TLS 1.3 and others there is possibly a security flaw using ALPN [1].

It seems to me that the ALPN protocol can be bypassed if the client does
not correctly inform the ClientHello header.

So, the suggestion is to check the ClientHello header in the server and
terminate the TLS handshake early.

Patch attached.

best regards,
Ranier Vilela

[1] terminate-tlsv1-3-handshake-if-alpn-is-missing



terminate-tls-handshake-if-no-alpn.patch
Description: Binary data

CVE's addressed in next update

[Mark Hill]

Do we know, is it posted anywhere on the postgresql.org site what CVE's will be 
addressed in the next round up updates
to Postgres which should come out next Thursday, May 9th, 2024?

Thanks, Mark

Re: CVE's addressed in next update

[Tom Lane]

Mark Hill  writes:
> Do we know, is it posted anywhere on the postgresql.org site what CVE's will 
> be addressed in the next round up updates
> to Postgres which should come out next Thursday, May 9th, 2024?

We do not announce that sort of thing in advance.

regards, tom lane

Re: Direct SSL connection and ALPN loose ends

[Heikki Linnakangas]

On 29/04/2024 20:10, Ranier Vilela wrote:

Hi,

With TLS 1.3 and others there is possibly a security flaw using ALPN [1].

It seems to me that the ALPN protocol can be bypassed if the client does 
not correctly inform the ClientHello header.


So, the suggestion is to check the ClientHello header in the server and
terminate the TLS handshake early.


Sounds to me like it's working as designed. ALPN in general is optional; 
if the client doesn't request it, then you proceed without it. We do 
require ALPN for direct SSL connections though. We can, because direct 
SSL connections is a new feature in Postgres. But we cannot require it 
for the connections negotiated with SSLRequest, or we break 
compatibility with old clients that don't use ALPN.


There is a check in direct SSL mode that ALPN was used 
(ProcessSSLStartup in backend_startup.c):



if (!port->alpn_used)
{
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("received direct SSL connection request 
without ALPN protocol negotiation extension")));
goto reject;
}


That happens immediately after the SSL connection has been established.

Hmm. I guess it would be better to abort the connection earlier, without 
completing the TLS handshake. Otherwise the client might send the first 
message in wrong protocol to the PostgreSQL server. That's not a 
security issue for the PostgreSQL server: the server disconnects without 
reading the message. And I don't see any way for an ALPACA attack when 
the server ignores the client's message. Nevertheless, from the point of 
view of keeping the attack surface as small as possible, aborting 
earlier seems better.


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

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Alexander Lakhin]

Hi Dmitry,

19.04.2024 02:26, Dmitry Koval wrote:


18.04.2024 19:00, Alexander Lakhin wrote:

leaves a strange constraint:
\d+ t*
   Table "public.tp_0"
...
Not-null constraints:
 "merge-16385-26BCB0-tmp_i_not_null" NOT NULL "i"


Thanks!
Attached fix (with test) for this case.
The patch should be applied after patches
v6-0001- ... .patch ... v6-0004- ... .patch


I still wonder, why that constraint (now with a less questionable name) is
created during MERGE?

That is, before MERGE, two partitions have only PRIMARY KEY indexes,
with no not-null constraint, and you can manually remove the constraint
after MERGE, so maybe it's not necessary...

Best regards,
Alexander

Re: Direct SSL connection with ALPN and HBA rules

[Jacob Champion]

On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas  wrote:
> I finally understood what you mean. So if the client supports ALPN, but
> the list of protocols that it provides does not include 'postgresql',
> the server should reject the connection with 'no_applicaton_protocol'
> alert.

Right. (And additionally, we reject clients that don't advertise ALPN
over direct SSL, also during the TLS handshake.)

> The attached patch makes that change. I used the alpn_cb() function in
> openssl's own s_server program as example for that.

This patch as written will apply the new requirement to the old
negotiation style, though, won't it? My test suite sees a bunch of
failures with that.

> Unfortunately the error message you got in the client with that was
> horrible (I modified the server to not accept the 'postgresql' protocol):
>
> psql "dbname=postgres sslmode=require host=localhost"
> psql: error: connection to server at "localhost" (::1), port 5432
> failed: SSL error: SSL error code 167773280



I filed a bug upstream [1].

Thanks,
--Jacob

[1] https://github.com/openssl/openssl/issues/24300

Re: Direct SSL connection and ALPN loose ends

[Ranier Vilela]

Em seg., 29 de abr. de 2024 às 14:56, Heikki Linnakangas 
escreveu:

> On 29/04/2024 20:10, Ranier Vilela wrote:
> > Hi,
> >
> > With TLS 1.3 and others there is possibly a security flaw using ALPN [1].
> >
> > It seems to me that the ALPN protocol can be bypassed if the client does
> > not correctly inform the ClientHello header.
> >
> > So, the suggestion is to check the ClientHello header in the server and
> > terminate the TLS handshake early.
>
> Sounds to me like it's working as designed. ALPN in general is optional;
> if the client doesn't request it, then you proceed without it. We do
> require ALPN for direct SSL connections though. We can, because direct
> SSL connections is a new feature in Postgres. But we cannot require it
> for the connections negotiated with SSLRequest, or we break
> compatibility with old clients that don't use ALPN.
>
Ok.
But what if I have a server configured for TLS 1.3 and that requires ALPN
to allow access?
What about a client configured without ALPN requiring connection?


>
> There is a check in direct SSL mode that ALPN was used
> (ProcessSSLStartup in backend_startup.c):
>
> > if (!port->alpn_used)
> > {
> > ereport(COMMERROR,
> > (errcode(ERRCODE_PROTOCOL_VIOLATION),
> >  errmsg("received direct SSL connection
> request without ALPN protocol negotiation extension")));
> > goto reject;
> > }
>
> That happens immediately after the SSL connection has been established.
>
> Hmm. I guess it would be better to abort the connection earlier, without
> completing the TLS handshake. Otherwise the client might send the first
> message in wrong protocol to the PostgreSQL server. That's not a
> security issue for the PostgreSQL server: the server disconnects without
> reading the message. And I don't see any way for an ALPACA attack when
> the server ignores the client's message. Nevertheless, from the point of
> view of keeping the attack surface as small as possible, aborting
> earlier seems better.
>
So the ClientHello callback is the correct way to determine the end.

best regards,
Ranier Vilela

Re: Possible to get LIMIT in an index access method?

[Matthias van de Meent]

On Mon, 29 Apr 2024 at 18:17, Chris Cleveland
 wrote:
>
> I'm developing an index access method.
>
> SELECT *
> FROM foo
> WHERE col <=> constant
> ORDER BY col <==> constant
> LIMIT 10;
>
> I'm successfully getting the WHERE and the ORDER BY clauses in my beginscan() 
> method. Is there any way to get the LIMIT (or OFFSET, for that matter)?

No, that is not possible.
The index AM does not know (*should* not know) about the visibility
state of indexed entries, except in those cases where the indexed
entries are dead to all running transactions. Additionally, it can't
(shouldn't) know about filters on columns that are not included in the
index. As such, pushing down limits into the index AM is only possible
in situations where you know that the table is fully visible (which
can't be guaranteed at runtime) and that no other quals on the table's
columns exist (which is possible, but unlikely to be useful).

GIN has one "solution" to this when you enable gin_fuzzy_search_limit
(default: disabled), where it throws an error if you try to extract
more results from the resultset after it's been exhausted while the AM
knows more results could exist.

> My access method is designed such that you have to fetch the entire result 
> set in one go. It's not streaming, like most access methods. As such, it 
> would be very helpful to know up front how many items I need to fetch from 
> the index.

Sorry, but I don't think we can know in advance how many tuples are
going to be extracted from an index scan.


Kind regards,

Matthias van de Meent.

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Daniel Gustafsson]

> On 28 Apr 2024, at 20:52, Tom Lane  wrote:
> 
> I wrote:
>> Here's a draft patch that attacks that.  It seems to fix the
>> problem with test_pg_dump: no dangling pg_init_privs grants
>> are left behind.

Reading this I can't find any sharp edges, and I prefer your changes to
recordExtensionInitPriv over the version I had half-baked by the time you had
this finished.  Trying to break it with the testcases I had devised also
failed, so +1.

> Here's a v2 that attempts to add some queries to test_pg_dump.sql
> to provide visual verification that pg_shdepend and pg_init_privs
> are updated correctly during DROP OWNED BY.  It's a little bit
> nasty to look at the ACL column of pg_init_privs, because that text
> involves the bootstrap superuser's name which is site-dependent.
> What I did to try to make the test stable is
> 
>  replace(initprivs::text, current_user, 'postgres') AS initprivs

Maybe that part warrants a small comment in the testfile to keep it from
sending future readers into rabbitholes?

> This is of course not bulletproof: with a sufficiently weird
> bootstrap superuser name, we could get false matches to parts
> of "regress_dump_test_role" or to privilege strings.  That
> seems unlikely enough to live with, but I wonder if anybody has
> a better idea.

I think that will be bulletproof enough to keep it working in the buildfarm and
among 99% of hackers.

--
Daniel Gustafsson

Re: Direct SSL connection and ALPN loose ends

[Heikki Linnakangas]

On 29/04/2024 21:06, Ranier Vilela wrote:
Em seg., 29 de abr. de 2024 às 14:56, Heikki Linnakangas 
mailto:hlinn...@iki.fi>> escreveu:


On 29/04/2024 20:10, Ranier Vilela wrote:
 > Hi,
 >
 > With TLS 1.3 and others there is possibly a security flaw using
ALPN [1].
 >
 > It seems to me that the ALPN protocol can be bypassed if the
client does
 > not correctly inform the ClientHello header.
 >
 > So, the suggestion is to check the ClientHello header in the
server and
 > terminate the TLS handshake early.

Sounds to me like it's working as designed. ALPN in general is
optional;
if the client doesn't request it, then you proceed without it. We do
require ALPN for direct SSL connections though. We can, because direct
SSL connections is a new feature in Postgres. But we cannot require it
for the connections negotiated with SSLRequest, or we break
compatibility with old clients that don't use ALPN.

Ok.
But what if I have a server configured for TLS 1.3 and that requires 
ALPN to allow access?

What about a client configured without ALPN requiring connection?


Sorry, I don't understand the questions. What about them?

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

Re: Direct SSL connection with ALPN and HBA rules

[Jacob Champion]

On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas  wrote:
> Sure, we'd try to fix them ASAP. But we've had the SSLRequest
> negotiation since time immemorial. If a new vulnerability is found, it's
> unlikely that we'd need to disable the SSLRequest negotiation altogether
> to fix it. We'd be in serious trouble with back-branches in that case.
> There's no sudden need to have a kill-switch for it.

I'm not really arguing that you'd need the kill switch to fix a
problem in the code. (At least, I'm not arguing that in this thread; I
reserve the right to argue that in the future. :D) But between the
point of time that a vulnerability is announced and a user has
upgraded, it's really nice to have a switch as a mitigation. Even
better if it's server-side, because then the DBA can protect all their
clients without requiring action on their part.

> Taking that to the extreme, you could argue for a kill-switch for every
> feature, just in case there's a vulnerability in them. I agree that
> authentication is more sensitive so reducing the surface of that is more
> reasonable. And but nevertheless.

I mean... that would be extreme, yeah. I don't think anyone's proposed that.

> If you only
> have v17 servers in your environment, so you know all servers support
> direct negotiation if they support SSL at all, but a mix of servers with
> and without SSL, sslnegotiation=directonly would reduce roundtrips with
> sslmode=prefer.

But if you're in that situation, what does the use of directonly give
you over `sslnegotiation=direct`? You already know that servers
support direct, so there's no additional performance penalty from the
less strict mode.

> Making requiredirect to imply sslmode=require, or error out unless you
> also set sslmode=require, feels like a cavalier way of forcing SSL. We
> should have a serious discussion on making sslmode=require the default
> instead. That would be a more direct way of nudging people to use SSL.
> It would cause a lot of breakage, but it would also be a big improvement
> to security.
>
> Consider how sslnegotiation=requiredirect/directonly would feel, if we
> made sslmode=require the default. If you explicitly set "sslmode=prefer"
> or "sslmode=disable", it would be annoying if you would also need to
> remove "sslnegotiation=requiredirect" from your connection string.

That's similar to how sslrootcert=system already works. To me, it
feels great, because I don't have to worry about nonsensical
combinations (with the exception of GSS, which we've touched on
above). libpq complains loudly if I try to shoot myself in the foot,
and if I'm using sslrootcert=system then it's a pretty clear signal
that I care more about security than the temporary inconvenience of
editing my connection string for one weird server that doesn't use SSL
for some reason.

> I think the best way forward for those is something like a new
> "require_proto" parameter that you suggested upthread. Perhaps call it
> "encryption", with options "none", "ssl", "gss" so that you can provide
> multiple options and libpq will try them in the order specified. For
> example:
>
> encryption=none
> encryption=ssl, none  # like sslmode=prefer
> encryption=gss
> encryption=gss, ssl   # try GSS first, then SSL
> encryption=ssl, gss   # try SSL first, then GSS
>
> This would make gssencmode and sslmode=disable/allow/prefer/require
> settings obsolete. sslmode would stil be needed to distinguish between
> verify-ca/verify-full though. But sslnegotiation would be still
> orthogonal to that.

I will give this some more thought.

Thanks,
--Jacob

Re: Direct SSL connection with ALPN and HBA rules

[Heikki Linnakangas]

On 29/04/2024 21:04, Jacob Champion wrote:

On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas  wrote:

I finally understood what you mean. So if the client supports ALPN, but
the list of protocols that it provides does not include 'postgresql',
the server should reject the connection with 'no_applicaton_protocol'
alert.


Right. (And additionally, we reject clients that don't advertise ALPN
over direct SSL, also during the TLS handshake.)


The attached patch makes that change. I used the alpn_cb() function in
openssl's own s_server program as example for that.


This patch as written will apply the new requirement to the old
negotiation style, though, won't it? My test suite sees a bunch of
failures with that.


Yes, and that is what we want, right? If the client uses old negotiation 
style, and includes ALPN in its ClientHello, but requests protocol 
"noodles" instead of "postgresql", it seems good to reject the connection.


Note that if the client does not request ALPN at all, the callback is 
not called, and the connection is accepted. Old clients still work 
because they do not request ALPN.



Unfortunately the error message you got in the client with that was
horrible (I modified the server to not accept the 'postgresql' protocol):

psql "dbname=postgres sslmode=require host=localhost"
psql: error: connection to server at "localhost" (::1), port 5432
failed: SSL error: SSL error code 167773280




I filed a bug upstream [1].


Thanks!

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

Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

[Maksim Milyutin]

On 14.04.2024 21:16, Maksim Milyutin wrote:


On 07.04.2024 02:07, Thomas Munro wrote:


So this is the version I plan to commit.

+bool
+EvictUnpinnedBuffer(Buffer buf)
+{
...
+/* This will return false if it becomes dirty or someone else pins it. */
+result = InvalidateVictimBuffer(desc);
+
+UnpinBuffer(desc);
+
+return result;
+}



Hi, Thomas!

Should not we call at the end the StrategyFreeBuffer() function to add 
target buffer to freelist and not miss it after invalidation?




Hello everyone!

Please take a look at this issue, current implementation of 
EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost 
permanently and will not be reused again


--
Best regards,
Maksim Milyutin

Re: Direct SSL connection with ALPN and HBA rules

[Jacob Champion]

On Mon, Apr 29, 2024 at 11:43 AM Heikki Linnakangas  wrote:
> Note that if the client does not request ALPN at all, the callback is
> not called, and the connection is accepted. Old clients still work
> because they do not request ALPN.

Ugh, sorry for the noise -- I couldn't figure out why all my old
clients were failing and then realized it was because I'd left some
test code in place for the OpenSSL bug. I'll rebuild everything and
keep reviewing.

--Jacob

Re: Virtual generated columns

[Corey Huinker]

On Mon, Apr 29, 2024 at 4:24 AM Peter Eisentraut 
wrote:

> Here is a patch set to implement virtual generated columns.
>

I'm very excited about this!



> The main feature patch (0005 here) generally works but has a number of
> open corner cases that need to be thought about and/or fixed, many of
> which are marked in the code or the tests.  I'll continue working on
> that.  But I wanted to see if I can get some feedback on the test
> structure, so I don't have to keep changing it around later.
>

I'd be very interested to see virtual generated columns working, as one of
my past customers had a need to reclassify data in a partitioned table, and
the ability to detach a partition, alter the virtual generated columns, and
re-attach would have been great. In case you care, it was basically an
"expired" flag, but the rules for what data "expired" varied by country of
customer and level of service.

+ * Stored generated columns cannot work: They are computed after
+ * BEFORE triggers, but partition routing is done before all
+ * triggers.  Maybe virtual generated columns could be made to
+ * work, but then they would need to be handled as an expression
+ * below.

I'd say you nailed it with the test structure. The stored/virtual
copy/split is the ideal way to approach this, which makes the diff very
easy to understand.

+1 for not handling domain types yet.

 -- generation expression must be immutable
-CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) STORED);
+CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) VIRTUAL);


Does a VIRTUAL generated column have to be immutable? I can see where the
STORED one has to be, but consider the following:

CREATE TABLE foo (
created_at timestamptz DEFAULT CURRENT_TIMESTAMP,
row_age interval GENERATED ALWAYS AS CURRENT_TIMESTAMP - created_at
);


 -- can't have generated column that is a child of normal column
 CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
STORED) INHERITS (gtest_normal);  -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
VIRTUAL) INHERITS (gtest_normal);  -- error


This is the barrier to the partitioning reorganization scheme I described
above. Is there any hard rule why a child table couldn't have a generated
column matching the parent's regular column? I can see where it might
prevent indexing that column on the parent table, but is there some other
dealbreaker or is this just a "it doesn't work yet" situation?

One last thing to keep in mind is that there are two special case
expressions in the spec:

GENERATED ALWAYS AS ROW START
GENERATED ALWAYS AS ROW END


and we'll need to be able to fit those into the catalog. I'll start another
thread for that unless you prefer I keep it here.

Re: Direct SSL connection with ALPN and HBA rules

[Heikki Linnakangas]

On 29/04/2024 21:43, Jacob Champion wrote:

On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas  wrote:

If you only
have v17 servers in your environment, so you know all servers support
direct negotiation if they support SSL at all, but a mix of servers with
and without SSL, sslnegotiation=directonly would reduce roundtrips with
sslmode=prefer.


But if you're in that situation, what does the use of directonly give
you over `sslnegotiation=direct`? You already know that servers
support direct, so there's no additional performance penalty from the
less strict mode.


Well, by that argument we don't need requiredirect/directonly at all. 
This goes back to whether it's a security feature or a performance feature.


There is a small benefit with sslmode=prefer if you connect to a server 
that doesn't support SSL, though. With sslnegotiation=direct, if the 
server rejects the direct SSL connection, the client will reconnect and 
try SSL with SSLRequest. The server will respond with 'N', and the 
client will proceed without encryption. sslnegotiation=directonly 
removes that SSLRequest attempt, eliminating one roundtrip.



Making requiredirect to imply sslmode=require, or error out unless you
also set sslmode=require, feels like a cavalier way of forcing SSL. We
should have a serious discussion on making sslmode=require the default
instead. That would be a more direct way of nudging people to use SSL.
It would cause a lot of breakage, but it would also be a big improvement
to security.

Consider how sslnegotiation=requiredirect/directonly would feel, if we
made sslmode=require the default. If you explicitly set "sslmode=prefer"
or "sslmode=disable", it would be annoying if you would also need to
remove "sslnegotiation=requiredirect" from your connection string.


That's similar to how sslrootcert=system already works. To me, it
feels great, because I don't have to worry about nonsensical
combinations (with the exception of GSS, which we've touched on
above). libpq complains loudly if I try to shoot myself in the foot,
and if I'm using sslrootcert=system then it's a pretty clear signal
that I care more about security than the temporary inconvenience of
editing my connection string for one weird server that doesn't use SSL
for some reason.


Oh I was not aware sslrootcert=system works like that. That's a bit 
surprising, none of the other ssl-related settings imply or require that 
SSL is actually used. Did we intend to set a precedence for new settings 
with that?


(adding Daniel in case he has an opinion)

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

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Daniel Gustafsson  writes:
>> On 28 Apr 2024, at 20:52, Tom Lane  wrote:
>> ... It's a little bit
>> nasty to look at the ACL column of pg_init_privs, because that text
>> involves the bootstrap superuser's name which is site-dependent.
>> What I did to try to make the test stable is
>> replace(initprivs::text, current_user, 'postgres') AS initprivs

> Maybe that part warrants a small comment in the testfile to keep it from
> sending future readers into rabbitholes?

Agreed.

>> This is of course not bulletproof: with a sufficiently weird
>> bootstrap superuser name, we could get false matches to parts
>> of "regress_dump_test_role" or to privilege strings.  That
>> seems unlikely enough to live with, but I wonder if anybody has
>> a better idea.

> I think that will be bulletproof enough to keep it working in the buildfarm 
> and
> among 99% of hackers.

It occurred to me to use "aclexplode" to expand the initprivs, and
then we can substitute names with simple equality tests.  The test
query is a bit more complicated, but I feel better about it.

v3 attached also has a bit more work on code comments.

regards, tom lane

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$:&l
  
 
 
+
+ SHARED_DEPENDENCY_INITACL (i)
+ 
+  
+   The referenced object (which must be a role) is mentioned in a
+   pg_init_privs
+   entry for the dependent object.
+   (A SHARED_DEPENDENCY_INITACL entry is not made for
+   the owner of the object, since the owner will have
+   a SHARED_DEPENDENCY_OWNER entry anyway.)
+  
+ 
+
+
 
  SHARED_DEPENDENCY_POLICY (r)
  
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..e6cc720579 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
    AclMode mask, AclMaskHow how,
    bool *is_missing);
 static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
-	Acl *new_acl);
+	Oid ownerId, Acl *new_acl);
 static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
-		  Acl *new_acl);
+		  Oid ownerId, Acl *new_acl);
 
 
 /*
@@ -1447,7 +1447,19 @@ SetDefaultACL(InternalDefaultACL *iacls)
 /*
  * RemoveRoleFromObjectACL
  *
- * Used by shdepDropOwned to remove mentions of a role in ACLs
+ * Used by shdepDropOwned to remove mentions of a role in ACLs.
+ *
+ * Notice that this doesn't accept an objsubid parameter, which is a bit bogus
+ * since the pg_shdepend record that caused us to call it certainly had one.
+ * If, for example, pg_shdepend records the existence of a permission on
+ * mytable.mycol, this function will effectively issue a REVOKE ALL ON TABLE
+ * mytable.  That gets the job done because (per SQL spec) such a REVOKE also
+ * revokes per-column permissions.  We could not recreate a situation where
+ * the role has table-level but not column-level permissions; but it's okay
+ * (for now anyway) because this is only used when we're dropping the role
+ * and so all its permissions everywhere must go away.  At worst it's a bit
+ * inefficient if the role has column permissions on several columns of the
+ * same table.
  */
 void
 RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
@@ -1790,7 +1802,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
 		CatalogTupleUpdate(attRelation, &newtuple->t_self, newtuple);
 
 		/* Update initial privileges for extensions */
-		recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+		recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
 ACL_NUM(new_acl) > 0 ? new_acl : NULL);
 
 		/* Update the shared dependency ACL info */
@@ -2050,7 +2062,8 @@ ExecGrant_Relation(InternalGrant *istmt)
 			CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
 
 			/* Update initial privileges for extensions */
-			recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+			recordExtensionInitPriv(relOid, RelationRelationId, 0,
+	ownerId, new_acl);
 
 			/* Update the shared dependency ACL info */
 			updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2264,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
 		CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
 
 		/* Update initial privileges for extensions */
-		recordExtensionInitPriv(objectid, classid, 0, new_acl);
+		recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
 
 		/* Update the shared dependency ACL info */
 		updateAclDependencies(classid,
@@ -2403,7 +2416,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
 		CatalogTupleUpdate(relation, &newtuple->t_self, newtuple)

Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

[Thomas Munro]

On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin  wrote:
>> Should not we call at the end the StrategyFreeBuffer() function to add 
>> target buffer to freelist and not miss it after invalidation?

> Please take a look at this issue, current implementation of 
> EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently 
> and will not be reused again

Hi Maksim,

Oops, thanks, will fix.

Re: Direct SSL connection and ALPN loose ends

[Ranier Vilela]

Em seg., 29 de abr. de 2024 às 15:36, Heikki Linnakangas 
escreveu:

> On 29/04/2024 21:06, Ranier Vilela wrote:
> > Em seg., 29 de abr. de 2024 às 14:56, Heikki Linnakangas
> > mailto:hlinn...@iki.fi>> escreveu:
> >
> > On 29/04/2024 20:10, Ranier Vilela wrote:
> >  > Hi,
> >  >
> >  > With TLS 1.3 and others there is possibly a security flaw using
> > ALPN [1].
> >  >
> >  > It seems to me that the ALPN protocol can be bypassed if the
> > client does
> >  > not correctly inform the ClientHello header.
> >  >
> >  > So, the suggestion is to check the ClientHello header in the
> > server and
> >  > terminate the TLS handshake early.
> >
> > Sounds to me like it's working as designed. ALPN in general is
> > optional;
> > if the client doesn't request it, then you proceed without it. We do
> > require ALPN for direct SSL connections though. We can, because
> direct
> > SSL connections is a new feature in Postgres. But we cannot require
> it
> > for the connections negotiated with SSLRequest, or we break
> > compatibility with old clients that don't use ALPN.
> >
> > Ok.
> > But what if I have a server configured for TLS 1.3 and that requires
> > ALPN to allow access?
> > What about a client configured without ALPN requiring connection?
>
> Sorry, I don't understand the questions. What about them?
>
Sorry, I'll try to be clearer.
The way it is designed, can we impose TLS 1.3 and ALPN to allow access to a
public server?

And if on the other side we have a client, configured without ALPN,
when requesting access, the server will refuse?

best regards,
Ranier Vilela

Re: Direct SSL connection with ALPN and HBA rules

[Jacob Champion]

On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas  wrote:
> On 29/04/2024 21:43, Jacob Champion wrote:
> > But if you're in that situation, what does the use of directonly give
> > you over `sslnegotiation=direct`? You already know that servers
> > support direct, so there's no additional performance penalty from the
> > less strict mode.
>
> Well, by that argument we don't need requiredirect/directonly at all.
> This goes back to whether it's a security feature or a performance feature.

That's what I've been trying to argue, yeah. If it's not a security
feature... why's it there?

> There is a small benefit with sslmode=prefer if you connect to a server
> that doesn't support SSL, though. With sslnegotiation=direct, if the
> server rejects the direct SSL connection, the client will reconnect and
> try SSL with SSLRequest. The server will respond with 'N', and the
> client will proceed without encryption. sslnegotiation=directonly
> removes that SSLRequest attempt, eliminating one roundtrip.

Okay, agreed that in this case, there is a performance benefit. It's
not enough to convince me, honestly, but are there any other cases I
missed as well?

> Oh I was not aware sslrootcert=system works like that. That's a bit
> surprising, none of the other ssl-related settings imply or require that
> SSL is actually used.

For sslrootcert=system in particular, the danger of accidentally weak
sslmodes is pretty high, especially for verify-ca mode. (It goes back
to that other argument -- there should be, effectively, zero users who
both opt in to the public CA system, and are also okay with silently
falling back and not using it.)

> Did we intend to set a precedence for new settings
> with that?

(I'll let committers answer whether they intended that or not -- I was
just bringing up that we already have a setting that works like that,
and I really like how it works in practice. But it's probably
unsurprising that I like it.)

--Jacob

Re: Direct SSL connection with ALPN and HBA rules

[Jacob Champion]

On Mon, Apr 29, 2024 at 12:32 PM Jacob Champion
 wrote:
>
> On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas  wrote:
> > On 29/04/2024 21:43, Jacob Champion wrote:
> > > But if you're in that situation, what does the use of directonly give
> > > you over `sslnegotiation=direct`? You already know that servers
> > > support direct, so there's no additional performance penalty from the
> > > less strict mode.
> >
> > Well, by that argument we don't need requiredirect/directonly at all.
> > This goes back to whether it's a security feature or a performance feature.
>
> That's what I've been trying to argue, yeah. If it's not a security
> feature... why's it there?

Er, I should clarify this. I _want_ requiredirect. I just want it to
be a security feature.

--Jacob

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Daniel Gustafsson]

> On 29 Apr 2024, at 21:15, Tom Lane  wrote:

> It occurred to me to use "aclexplode" to expand the initprivs, and
> then we can substitute names with simple equality tests.  The test
> query is a bit more complicated, but I feel better about it.

Nice, I didn't even remember that function existed.  I agree that it's an
improvement even at the increased query complexity.

> v3 attached also has a bit more work on code comments.

LGTM.

--
Daniel Gustafsson

Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

[Thomas Munro]

On Tue, Apr 30, 2024 at 7:17 AM Thomas Munro  wrote:
> On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin  wrote:
> >> Should not we call at the end the StrategyFreeBuffer() function to add 
> >> target buffer to freelist and not miss it after invalidation?
>
> > Please take a look at this issue, current implementation of 
> > EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost 
> > permanently and will not be reused again

I don't think that's true: it is not lost permanently, it'll be found
by the regular clock hand.  Perhaps it should be put on the freelist
so it can be found again quickly, but I'm not sure that's a bug, is
it?  If it were true, even basic testing eg select
count(pg_buffercache_evict(bufferid)) from pg_buffercache would leave
the system non-functional, but it doesn't, the usual CLOCK algorithm
just does its thing.

Re: TerminateOtherDBBackends code comments inconsistency.

[Noah Misch]

On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote:
> On Mon, Apr 22, 2024 at 9:56 PM Noah Misch  wrote:
> >
> > On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote:
> > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke  
> > > wrote:
> > > >
> > > > While working on [0] i have noticed this comment in
> > > > TerminateOtherDBBackends function:
> > > >
> > > > /*
> > > > * Check whether we have the necessary rights to terminate other
> > > > * sessions. We don't terminate any session until we ensure that we
> > > > * have rights on all the sessions to be terminated. These checks are
> > > > * the same as we do in pg_terminate_backend.
> > > > *
> > > > * In this case we don't raise some warnings - like "PID %d is not a
> > > > * PostgreSQL server process", because for us already finished session
> > > > * is not a problem.
> > > > */
> > > >
> > > > This statement is not true after 3a9b18b.
> > > > "These checks are the same as we do in pg_terminate_backend."
> >
> > The comment mismatch is a problem.  Thanks for reporting it.  The DROP
> > DATABASE doc mimics the comment, using, "permissions are the same as with
> > pg_terminate_backend".
> >
> 
> How about updating the comments as in the attached? I see that commit

I think the rationale for the difference should appear, being non-obvious and
debatable in this case.

> 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever
> is mentioned w.r.t permissions in the doc of that function sounds
> valid for drop database force to me. Do you have any specific proposal
> in your mind?

Something like the attached.  One could argue the function should also check
isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've
not done that.
Author: Noah Misch 
Commit: Noah Misch 



diff --git a/doc/src/sgml/ref/drop_database.sgml 
b/doc/src/sgml/ref/drop_database.sgml
index ff01450..9215b1a 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -79,12 +79,14 @@ DROP DATABASE [ IF EXISTS ] name [
   It doesn't terminate if prepared transactions, active logical replication
   slots or subscriptions are present in the target database.
  
+ 
  
-  This will fail if the current user has no permissions to terminate other
-  connections. Required permissions are the same as with
-  pg_terminate_backend, described in
-  .  This will also fail if we
-  are not able to terminate connections.
+  This terminates background worker connections and connections that the
+  current user has permission to terminate
+  with pg_terminate_backend, described in
+  .  If connections remain, this
+  command will fail.
  
 

diff --git a/src/backend/storage/ipc/procarray.c 
b/src/backend/storage/ipc/procarray.c
index 1a83c42..ab7651d 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3808,8 +3808,8 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int 
*nprepared)
  * The current backend is always ignored; it is caller's responsibility to
  * check whether the current backend uses the given DB, if it's important.
  *
- * It doesn't allow to terminate the connections even if there is a one
- * backend with the prepared transaction in the target database.
+ * If the target database has a prepared transaction or permissions checks
+ * fail for a connection, this fails without terminating anything.
  */
 void
 TerminateOtherDBBackends(Oid databaseId)
@@ -3854,14 +3854,19 @@ TerminateOtherDBBackends(Oid databaseId)
ListCell   *lc;
 
/*
-* Check whether we have the necessary rights to terminate other
-* sessions.  We don't terminate any session until we ensure 
that we
-* have rights on all the sessions to be terminated.  These 
checks are
-* the same as we do in pg_terminate_backend.
+* Permissions checks relax the pg_terminate_backend checks in 
two
+* ways, both by omitting the !OidIsValid(proc->roleId) check:
 *
-* In this case we don't raise some warnings - like "PID %d is 
not a
-* PostgreSQL server process", because for us already finished 
session
-* is not a problem.
+* - Accept terminating autovacuum workers, since DROP DATABASE
+*   without FORCE terminates them.
+*
+* - Accept terminating bgworkers.  For bgworker authors, it's
+*   convenient to be able to recommend FORCE if a worker is 
blocking
+*   DROP DATABASE unexpectedly.
+*
+* Unlike pg_terminate_backend, we don't raise some warnings - 
like
+* "PID %d is not a PostgreSQL server process", because for us 
already
+* finished session is not a problem.
 

Support tid range scan in parallel?

[Cary Huang]

Hello

 

When using ctid as a
restriction clause with lower and upper bounds, PostgreSQL's planner will use
TID range scan plan to handle such query. This works and generally fine.
However, if the ctid range covers a huge amount of data, the planner will not
use parallel worker to perform ctid range scan because it is not supported. It 
could, however,
still choose to use parallel sequential scan to complete the scan if ti costs 
less. 

 

In one of our
migration scenarios, we rely on tid range scan to migrate huge table from one
database to another once the lower and upper ctid bound is determined. With the
support of parallel ctid range scan, this process could be done much quicker.

 

The attached patch
is my approach to add parallel ctid range scan to PostgreSQL's planner and 
executor. In my
tests, I do see an increase in performance using parallel tid range scan over
the single worker tid range scan and it is also faster than parallel sequential
scan covering similar ranges. Of course, the table needs to be large enough to
reflect the performance increase.

 

below is the timing to complete a select query covering all the records in a 
simple 2-column
table with 40 million records,

 

 - tid range scan takes 10216ms

 - tid range scan with 2 workers takes 7109ms

 - sequential scan with 2 workers takes 8499ms

 

Having the support
for parallel ctid range scan is definitely helpful in our migration case, I am
sure it could be useful in other cases as well. I am sharing the patch here and
if someone could provide a quick feedback or review that would be greatly 
appreciated.

 

Thank you!

 

Cary Huang

-

HighGo Software Inc. (Canada)

mailto:cary.hu...@highgo.ca

http://www.highgo.ca

v1-0001-add-parallel-tid-rangescan.patch
Description: Binary data

Re: Internal error codes triggered by tests

[Michael Paquier]

On Mon, Apr 29, 2024 at 08:02:45AM -0500, Justin Pryzby wrote:
> I sent a list of user-facing elogs here, a few times.
> ZDclRM/jate66...@telsasoft.com
> 
> And if someone had expressed an interest, I might have sent a longer
> list.

Thanks.  I'll take a look at what you have there.  Nothing would be
committed before v18 opens, though.
--
Michael


signature.asc
Description: PGP signature

Re: A failure in prepared_xacts test

[Michael Paquier]

On Mon, Apr 29, 2024 at 09:45:16AM -0400, Tom Lane wrote:
> TAP tests shouldn't be at risk, because there is no "make
> installcheck" equivalent for them.  Each TAP test creates its own
> database instance (or maybe several), so that instance won't have
> anything else going on.

There are a few more 2PC transactions in test_decoding (no
installcheck), temp.sql, test_extensions.sql and pg_stat_statements's
utility.sql (no installcheck) but their GIDs are not that bad.
twophase_stream.sql has a GID "test1", which is kind of generic, but
it won't run in parallel.  At the end, only addressing the
prepared_xacts.sql and the ECPG bits looked enough to me, so I've
tweaked these with 7e61e4cc7cfc and called it a day.

I'd be curious about any discussion involving the structure of the
meson tests.
--
Michael


signature.asc
Description: PGP signature

Re: Support a wildcard in backtrace_functions

[Michael Paquier]

On Mon, Apr 29, 2024 at 08:08:19AM -0400, Robert Haas wrote:
> On Mon, Apr 29, 2024 at 5:12 AM Peter Eisentraut  wrote:
>> Ok, it's reverted.

Thanks for taking care of it.

> Thanks, and sorry. :-(

Sorry for the outcome..
--
Michael


signature.asc
Description: PGP signature

Re: Support tid range scan in parallel?

[David Rowley]

On Tue, 30 Apr 2024 at 10:36, Cary Huang  wrote:
> In one of our migration scenarios, we rely on tid range scan to migrate huge 
> table from one database to another once the lower and upper ctid bound is 
> determined. With the support of parallel ctid range scan, this process could 
> be done much quicker.

I would have thought that the best way to migrate would be to further
divide the TID range into N segments and run N queries, one per
segment to get the data out.

>From a CPU point of view, I'd hard to imagine that a SELECT * query
without any other items in the WHERE clause other than the TID range
quals would run faster with multiple workers than with 1.  The problem
is the overhead of pushing tuples to the main process often outweighs
the benefits of the parallelism.  However, from an I/O point of view
on a server with slow enough disks, I can imagine there'd be a
speedup.

> The attached patch is my approach to add parallel ctid range scan to 
> PostgreSQL's planner and executor. In my tests, I do see an increase in 
> performance using parallel tid range scan over the single worker tid range 
> scan and it is also faster than parallel sequential scan covering similar 
> ranges. Of course, the table needs to be large enough to reflect the 
> performance increase.
>
> below is the timing to complete a select query covering all the records in a 
> simple 2-column table with 40 million records,
>
>  - tid range scan takes 10216ms
>  - tid range scan with 2 workers takes 7109ms
>  - sequential scan with 2 workers takes 8499ms

Can you share more details about this test? i.e. the query, what the
times are that you've measured (EXPLAIN ANALYZE, or SELECT, COPY?).
Also, which version/commit did you patch against?  I was wondering if
the read stream code added in v17 would result in the serial case
running faster because the parallelism just resulted in more I/O
concurrency.

Of course, it may be beneficial to have parallel TID Range for other
cases when more row filtering or aggregation is being done as that
requires pushing fewer tuples over from the parallel worker to the
main process. It just would be good to get to the bottom of if there's
still any advantage to parallelism when no filtering other than the
ctid quals is being done now that we've less chance of having to wait
for I/O coming from disk with the read streams code.

David

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Daniel Gustafsson  writes:
> On 29 Apr 2024, at 21:15, Tom Lane  wrote:
>> v3 attached also has a bit more work on code comments.

> LGTM.

Pushed, thanks for reviewing!

regards, tom lane

Re: Logging which interface was connected to in log_line_prefix

[Cary Huang]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Hi 

I did a quick test on this patch and it seems to work as expected. Originally I 
thought the patch would add the name of "local interface" such as "eth0", 
"eth1", "lo"... etc as %L log prefix format. Instead, it formats the local 
interface IP addresses , but I think it is fine too. 

I have tested this new addition with various types of IPs including IPv4, IPv4 
and IPv6 local loop back addresses, global IPv6 address, linked local IPv6 
address with interface specifier, it seems to format these IPs correctly

There is a comment in the patch that states:

/* We do not need clean_ipv6_addr here: just report verbatim */

I am not quite sure what it means, but I am guessing it means that the patch 
does not need to format the IPv6 addresses in any specific way. For example, 
removing leading zeros or compressing consecutive zeros to make a IPv6 address 
shorter. It may not be necessary to indicate this in a comment because In my 
test, if any of my interface's IPv6 address have consecutive zeroes like this: 
2000::::::200:cafe/64, my network driver (Ubuntu 18.04) 
will format it as 2000::200:cafe, and the patch of course will read it as 
2000::200:cafe, which is ... correct and clean.

thank you
Cary Huang
www.highgo.ca

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

I wrote:
> Pushed, thanks for reviewing!

Argh, I forgot I'd meant to push b0c5b215d first not second.
Oh well, it was only neatnik-ism that made me want to see
those other animals fail --- and a lot of the buildfarm is
red right now for $other_reasons anyway.

regards, tom lane

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Dmitry Koval]

Hi!

1.
29.04.2024 21:00, Alexander Lakhin wrote:

I still wonder, why that constraint (now with a less questionable name) is
created during MERGE?


The SPLIT/MERGE PARTITION(S) commands for creating partitions reuse the 
existing code of CREATE TABLE .. LIKE ... command. A new partition was 
created with the name "merge-16385-26BCB0-tmp" (since there was an old 
partition with the same name). The constraint 
"merge-16385-26BCB0-tmp_i_not_null" was created too together with the 
partition. Subsequently, the table was renamed, but the constraint was not.
Now a new partition is immediately created with the correct name (the 
old partition is renamed).


2.
Just in case, I am attaching a small fix v9_fix.diff for situation [1].

[1] 
https://www.postgresql.org/message-id/0520c72e-8d97-245e-53f9-173beca2ab2e%40gmail.com


--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.comdiff --git a/src/backend/parser/parse_utilcmd.c 
b/src/backend/parser/parse_utilcmd.c
index fef084f5d5..e918a623c5 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3446,6 +3446,11 @@ checkPartition(Relation rel, Oid partRelOid)

RelationGetRelationName(partRel),
RelationGetRelationName(rel;
 
+   /* Permissions checks */
+   if (!object_ownercheck(RelationRelationId, RelationGetRelid(partRel), 
GetUserId()))
+   aclcheck_error(ACLCHECK_NOT_OWNER, 
get_relkind_objtype(partRel->rd_rel->relkind),
+  RelationGetRelationName(partRel));
+
relation_close(partRel, AccessShareLock);
 }
 

Re: A failure in prepared_xacts test

[Richard Guo]

On Mon, Apr 29, 2024 at 9:45 PM Tom Lane  wrote:

> Richard Guo  writes:
> > I noticed that some TAP tests from recovery and subscription would
> > select the count from pg_prepared_xacts.  I wonder if these tests would
> > be affected if there are any prepared transactions on the backend.
>
> TAP tests shouldn't be at risk, because there is no "make
> installcheck" equivalent for them.  Each TAP test creates its own
> database instance (or maybe several), so that instance won't have
> anything else going on.


Thank you for the explanation.  I wasn't aware of this before.

Thanks
Richard

jsonpath Time and Timestamp Special Cases

[David E. Wheeler]

Hello hackers,

I noticed that the jsonpath date/time functions (.time() and timestamp(), et 
al.) don’t support some valid but special-case PostgreSQL values, notably 
`infinity`, `-infinity`, and, for times, '24:00:00`:

❯ psql
psql (17devel)
Type "help" for help.

david=# select jsonb_path_query(to_jsonb('24:00:00'::time), '$.time()');
ERROR:  time format is not recognized: "24:00:00"

david=# select jsonb_path_query(to_jsonb('infinity'::timestamptz), 
'$.timestamp_tz()');
ERROR:  timestamp_tz format is not recognized: "infinity"

I assume this is because the standard doesn’t support these, or references 
JavaScript-only values or some such. Is that right?

Best,

David

Re: A failure in prepared_xacts test

[Richard Guo]

On Tue, Apr 30, 2024 at 6:43 AM Michael Paquier  wrote:

> I'd be curious about any discussion involving the structure of the
> meson tests.


+1.  I'm kind of worried that the expansion of parallelization could
lead to more instances of instability.  Alexander mentioned one such
case at [1].  I haven't looked into it though.

[1]
https://www.postgresql.org/message-id/cbf0156f-5aa1-91db-5802-82435dda03e6%40gmail.com

Thanks
Richard

Re: A failure in prepared_xacts test

[Tom Lane]

Richard Guo  writes:
> +1.  I'm kind of worried that the expansion of parallelization could
> lead to more instances of instability.  Alexander mentioned one such
> case at [1].  I haven't looked into it though.
> [1] 
> https://www.postgresql.org/message-id/cbf0156f-5aa1-91db-5802-82435dda03e6%40gmail.com

The mechanism there is pretty obvious: a plancache flush happened
at just the wrong (right?) time and caused the output to change,
as indeed the comment acknowledges:

 -- currently, this fails due to cached plan for "r.f1 + 1" expression
 -- (but if debug_discard_caches is on, it will succeed)

I wonder if we shouldn't just remove that test case as being
too unstable -- especially since it's not proving much anyway.

regards, tom lane

Re: Fix parallel vacuum buffer usage reporting

[Masahiko Sawada]

On Fri, Apr 26, 2024 at 9:12 PM Alena Rybakina  wrote:
>
> Hi!
>>
>> The same script was run, but using vacuum verbose analyze, and I saw the 
>> difference again in the fifth step:
>> with your patch: buffer usage: 32312 hits, 607 misses, 1566 dirtied
>> master: buffer usage: 32346 hits, 573 misses, 1360 dirtied
>
> Isn't there a chance for the checkpointer to run during this time? That could 
> make the conditions between the two runs slightly different and explain the 
> change in buffer report.
>
> [0] 
> https://github.com/postgres/postgres/blob/8a1b31e6e59631807a08a4e9465134c343bbdf5e/src/backend/access/heap/vacuumlazy.c#L2826-L2831
>
> Looking at the script, you won't trigger the problem.
>
> Thank you for the link I accounted it in my next experiments.
>
> I repeated the test without processing checkpoints with a single index, and 
> the number of pages in the buffer used almost matched:
>
> master branch: buffer usage: 32315 hits, 606 misses, 4486 dirtied
>
> with applied patch v4 version: buffer usage: 32315 hits, 606 misses, 4489 
> dirtied
>
> I think you are right - the problem was interfering with the checkpoint 
> process, by the way I checked the first version patch. To cut a long story 
> short, everything is fine now with one index.
>
> Just in case, I'll explain: I considered this case because your patch could 
> have impact influenced it too.
>
> On 25.04.2024 10:17, Anthonin Bonnefoy wrote:
>
>
> On Wed, Apr 24, 2024 at 4:01 PM Alena Rybakina  
> wrote:
>>
>> I tested the main postgres branch with and without your fix using a script 
>> that was written by me. It consists of five scenarios and I made a 
>> comparison in the logs between the original version of the master branch and 
>> the master branch with your patch:
>
>  Hi! Thanks for the tests.
>
>> I have attached a test file (vacuum_check_logs.sql)
>
> The reporting issue will only happen if there's a parallel index vacuum and 
> it will only happen if there's at least 2 indexes [0]. You will need to 
> create an additional index.
>
> Speaking of the problem, I added another index and repeated the test and 
> found a significant difference:
>
> I found it when I commited the transaction (3):
>
> master: 2964 hits, 0 misses, 0 dirtied
>
> with applied patch v4 version: buffer usage: 33013 hits, 0 misses, 3 dirtied
>
> When I deleted all the data from the table and later started vacuum verbose 
> again (4):
>
> master: buffer usage: 51486 hits, 0 misses, 0 dirtied
>
> with applied patch v4 version:buffer usage: 77924 hits, 0 misses, 0 dirtied
>
> when I inserted 1 million data into the table and updated it (5):
>
> master:buffer usage: 27904 hits, 5021 misses, 1777 dirtied
>
> with applied patch v4 version:buffer usage: 41051 hits, 9973 misses, 2564 
> dirtied
>
> As I see, the number of pages is significantly more than it was in the master 
> branch and ,frankly, I couldn't fully figure out if it was a mistake or not.

I think that the patch fixes the problem correctly.

I've run pgindent and updated the commit message. I realized that
parallel vacuum was introduced in pg13 but buffer usage reporting in
VACUUM command was implemented in pg15. Therefore, in pg13 and pg14,
VACUUM (PARALLEL) is available but VACUUM (PARALLEL, VERBOSE) doesn't
show the buffer usage report. Autovacuum does show the buffer usage
report but parallel autovacuum is not supported. Therefore, we should
backpatch it down to 15, not 13.

I'm going to push the patch down to pg15, barring any objections.

Regards,

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


v5-0001-Fix-parallel-vacuum-buffer-usage-reporting.patch
Description: Binary data

CREATE TABLE/ProcessUtility hook behavior change

[David Steele]

Hackers,

While testing pgAudit against PG17 I noticed the following behavioral 
change:


CREATE TABLE public.test
(
id INT,
name TEXT,
description TEXT,
CONSTRAINT test_pkey PRIMARY KEY (id)
);
NOTICE:  AUDIT: SESSION,23,1,DDL,CREATE TABLE,TABLE,public.test,"CREATE 
TABLE public.test

(
id INT,
name TEXT,
description TEXT,
CONSTRAINT test_pkey PRIMARY KEY (id)
);",
NOTICE:  AUDIT: SESSION,23,1,DDL,ALTER TABLE,TABLE,public.test,"CREATE 
TABLE public.test

(
id INT,
name TEXT,
description TEXT,
CONSTRAINT test_pkey PRIMARY KEY (id)
);",
NOTICE:  AUDIT: SESSION,23,1,DDL,CREATE 
INDEX,INDEX,public.test_pkey,"CREATE TABLE public.test

(
id INT,
name TEXT,
description TEXT,
CONSTRAINT test_pkey PRIMARY KEY (id)
);",

Prior to PG17, ProcessUtility was not being called for ALTER TABLE in 
this context. The change probably makes some sense, since the table is 
being created then altered to add the primary key, but since it is a 
behavioral change I wanted to bring it up.


I attempted a bisect to identify the commit that caused this behavioral 
change but pgAudit has been broken since at least November on master and 
didn't get fixed again until bb766cde (April 8). Looking at that commit 
I'm a bit baffled by how it fixed the issue I was seeing, which was that 
an event trigger was firing in the wrong context in the pgAudit tests:


 CREATE TABLE tmp (id int, data text);
+ERROR:  not fired by event trigger manager

That error comes out of pgAudit itself:

if (!CALLED_AS_EVENT_TRIGGER(fcinfo))
elog(ERROR, "not fired by event trigger manager");

Since bb766cde cannot be readily applied to older commits in master I'm 
unable to continue bisecting to find the ALTER TABLE behavioral change.


This seems to leave me with manual code inspection and there have been a 
lot of changes in this area, so I'm hoping somebody will know why this 
ALTER TABLE change happened before I start digging into it.


Regards,
-David

[PATCH] Fix bug when calling strncmp in check_authmethod_valid

[Jingxian Li]

Hi all,

Attached is a patch that fixes bug when calling strncmp function, in 
which case the third argument (authmethod - strchr(authmethod, ' ')) 
may be negative, which is not as expected..


With Regards,
Jingxian Li.




v1-0001-Fix-bug-when-calling-strncmp-in-check_authmethod_valid.patch
Description: Binary data

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Alexander Lakhin]

30.04.2024 03:10, Dmitry Koval wrote:

Hi!

1.
29.04.2024 21:00, Alexander Lakhin wrote:

I still wonder, why that constraint (now with a less questionable name) is
created during MERGE?


The SPLIT/MERGE PARTITION(S) commands for creating partitions reuse the existing code of CREATE TABLE .. LIKE ... 
command. A new partition was created with the name "merge-16385-26BCB0-tmp" (since there was an old partition with the 
same name). The constraint "merge-16385-26BCB0-tmp_i_not_null" was created too together with the partition. 
Subsequently, the table was renamed, but the constraint was not.

Now a new partition is immediately created with the correct name (the old 
partition is renamed).


Maybe I'm doing something wrong, but the following script:
CREATE TABLE t (i int, PRIMARY KEY(i)) PARTITION BY RANGE (i);
CREATE TABLE tp_0 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (1) TO (2);

CREATE TABLE t2 (LIKE t INCLUDING ALL);
CREATE TABLE tp2 (LIKE tp_0 INCLUDING ALL);
creates tables t2, tp2 without not-null constraints.

But after
ALTER TABLE t MERGE PARTITIONS (tp_0, tp_1) INTO tp_0;
I see:
\d+ tp_0
...
Indexes:
    "tp_0_pkey" PRIMARY KEY, btree (i)
Not-null constraints:
    "tp_0_i_not_null" NOT NULL "i"

Best regards,
Alexander

Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation

[Masahiko Sawada]

On Tue, Apr 23, 2024 at 11:57 PM Tom Lane  wrote:
>
> "Guo, Adam"  writes:
> > I would like to report an issue with the pg_trgm extension on
> > cross-architecture replication scenarios. When an x86_64 standby
> > server is replicating from an aarch64 primary server or vice versa,
> > the gist_trgm_ops opclass returns different results on the primary
> > and standby.
>
> I do not think that is a supported scenario.  Hash functions and
> suchlike are not guaranteed to produce the same results on different
> CPU architectures.  As a quick example, I get
>
> regression=# select hashfloat8(34);
>  hashfloat8
> 
>21570837
> (1 row)
>
> on x86_64 but
>
> postgres=# select hashfloat8(34);
>  hashfloat8
> 
>  -602898821
> (1 row)
>
> on ppc32 thanks to the endianness difference.
>
> > Given that this has problem has come up before and seems likely to
> > come up again, I'm curious what other broad solutions there might be
> > to resolve it?
>
> Reject as not a bug.  Discourage people from thinking that physical
> replication will work across architectures.

While cross-arch physical replication is not supported, I think having
architecture dependent differences is not good and It's legitimate to
fix it. FYI the 'char' data type comparison is done as though char is
unsigned. I've attached a small patch to fix it. What do you think?

Regards,

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


fix_signedness_issue_in_pg_trgm.patch
Description: Binary data

Re: CREATE TABLE/ProcessUtility hook behavior change

[jian he]

On Tue, Apr 30, 2024 at 10:26 AM David Steele  wrote:
>
> Hackers,
>
> While testing pgAudit against PG17 I noticed the following behavioral
> change:
>
> CREATE TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );
> NOTICE:  AUDIT: SESSION,23,1,DDL,CREATE TABLE,TABLE,public.test,"CREATE
> TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",
> NOTICE:  AUDIT: SESSION,23,1,DDL,ALTER TABLE,TABLE,public.test,"CREATE
> TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",
> NOTICE:  AUDIT: SESSION,23,1,DDL,CREATE
> INDEX,INDEX,public.test_pkey,"CREATE TABLE public.test
> (
> id INT,
> name TEXT,
> description TEXT,
> CONSTRAINT test_pkey PRIMARY KEY (id)
> );",
>
> Prior to PG17, ProcessUtility was not being called for ALTER TABLE in
> this context. The change probably makes some sense, since the table is
> being created then altered to add the primary key, but since it is a
> behavioral change I wanted to bring it up.
>
> I attempted a bisect to identify the commit that caused this behavioral
> change but pgAudit has been broken since at least November on master and
> didn't get fixed again until bb766cde (April 8). Looking at that commit
> I'm a bit baffled by how it fixed the issue I was seeing, which was that
> an event trigger was firing in the wrong context in the pgAudit tests:
>
>   CREATE TABLE tmp (id int, data text);
> +ERROR:  not fired by event trigger manager
>
> That error comes out of pgAudit itself:
>
>  if (!CALLED_AS_EVENT_TRIGGER(fcinfo))
>  elog(ERROR, "not fired by event trigger manager");
>
> Since bb766cde cannot be readily applied to older commits in master I'm
> unable to continue bisecting to find the ALTER TABLE behavioral change.
>
> This seems to leave me with manual code inspection and there have been a
> lot of changes in this area, so I'm hoping somebody will know why this
> ALTER TABLE change happened before I start digging into it.

probably this commit:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0cd711271d42b0888d36f8eda50e1092c2fed4b3

especially:

- * The parser will create AT_AttSetNotNull subcommands for
- * columns of PRIMARY KEY indexes/constraints, but we need
- * not do anything with them here, because the columns'
- * NOT NULL marks will already have been propagated into
- * the new table definition.
+ * We see this subtype when a primary key is created
+ * internally, for example when it is replaced with a new
+ * constraint (say because one of the columns changes
+ * type); in this case we need to reinstate attnotnull,
+ * because it was removed because of the drop of the old
+ * PK. Schedule this subcommand to an upcoming AT pass.
*/
+ cmd->subtype = AT_SetAttNotNull;
+ tab->subcmds[AT_PASS_OLD_COL_ATTRS] =
+ lappend(tab->subcmds[AT_PASS_OLD_COL_ATTRS], cmd);

Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation

[Tom Lane]

Masahiko Sawada  writes:
> On Tue, Apr 23, 2024 at 11:57 PM Tom Lane  wrote:
>> Reject as not a bug.  Discourage people from thinking that physical
>> replication will work across architectures.

> While cross-arch physical replication is not supported, I think having
> architecture dependent differences is not good and It's legitimate to
> fix it. FYI the 'char' data type comparison is done as though char is
> unsigned. I've attached a small patch to fix it. What do you think?

I think this will break existing indexes that are working fine.
Yeah, it would have been better to avoid the difference, but
it's too late now.

regards, tom lane

Re: TerminateOtherDBBackends code comments inconsistency.

[Amit Kapila]

On Tue, Apr 30, 2024 at 2:58 AM Noah Misch  wrote:
>
> On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote:
> > On Mon, Apr 22, 2024 at 9:56 PM Noah Misch  wrote:
>
> > 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever
> > is mentioned w.r.t permissions in the doc of that function sounds
> > valid for drop database force to me. Do you have any specific proposal
> > in your mind?
>
> Something like the attached.
>

LGTM.

>  One could argue the function should also check
> isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've
> not done that.

What is the argument for ignoring such workers?

-- 
With Regards,
Amit Kapila.

Re: Synchronizing slots from primary to standby

[shveta malik]

On Mon, Apr 29, 2024 at 5:28 PM Zhijie Hou (Fujitsu)
 wrote:
>
> On Monday, April 29, 2024 5:11 PM shveta malik  wrote:
> >
> > On Mon, Apr 29, 2024 at 11:38 AM shveta malik 
> > wrote:
> > >
> > > On Mon, Apr 29, 2024 at 10:57 AM Zhijie Hou (Fujitsu)
> > >  wrote:
> > > >
> > > > On Friday, March 15, 2024 10:45 PM Bertrand Drouvot
> >  wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Thu, Mar 14, 2024 at 02:22:44AM +, Zhijie Hou (Fujitsu) wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Since the standby_slot_names patch has been committed, I am
> > > > > > attaching the last doc patch for review.
> > > > > >
> > > > >
> > > > > Thanks!
> > > > >
> > > > > 1 ===
> > > > >
> > > > > +   continue subscribing to publications now on the new primary
> > > > > + server
> > > > > without
> > > > > +   any data loss.
> > > > >
> > > > > I think "without any data loss" should be re-worded in this
> > > > > context. Data loss in the sense "data committed on the primary and
> > > > > not visible on the subscriber in case of failover" can still occurs 
> > > > > (in case
> > synchronous replication is not used).
> > > > >
> > > > > 2 ===
> > > > >
> > > > > +   If the result (failover_ready) of both above 
> > > > > steps is
> > > > > +   true, existing subscriptions will be able to continue without data
> > loss.
> > > > > +  
> > > > >
> > > > > I don't think that's true if synchronous replication is not used.
> > > > > Say,
> > > > >
> > > > > - synchronous replication is not used
> > > > > - primary is not able to reach the standby anymore and
> > > > > standby_slot_names is set
> > > > > - new data is inserted into the primary
> > > > > - then not replicated to subscriber (due to standby_slot_names)
> > > > >
> > > > > Then I think the both above steps will return true but data would
> > > > > be lost in case of failover.
> > > >
> > > > Thanks for the comments, attach the new version patch which reworded
> > > > the above places.
> > >
> > > Thanks for the patch.
> > >
> > > Few comments:
> > >
> > > 1)  Tested the steps, one of the queries still refers to
> > > 'conflict_reason'. I think it should refer 'conflicting'.
>
> Thanks for catching this. Fixed.
>
> > >
> > > 2) Will it be good to mention that in case of planned promotion, it is
> > > recommended to run  pg_sync_replication_slots() as last sync attempt
> > > before we run failvoer-ready validation steps? This can be mentioned
> > > in high-availaibility.sgml of current patch
> >
> > I recall now that with the latest fix, we cannot run
> > pg_sync_replication_slots() unless we disable the slot-sync worker.
> > Considering that, I think it will be too many steps just to run the SQL 
> > function at
> > the end without much value added. Thus we can skip this point, we can rely 
> > on
> > slot sync worker completely.
>
> Agreed. I didn't change this.
>
> Here is the V3 doc patch.

Thanks for the patch.

It will be good if 1a can produce quoted slot-names list as output,
which can be used directly in step 1b's query; otherwise, it is little
inconvenient to give input to 1b if the number of slots are huge. User
needs to manually quote each slot-name.

Other than this, the patch looks good to me.

thanks
Shveta

Re: Help update PostgreSQL 13.12 to 13.14

[Kashif Zeeshan]

On Mon, Apr 29, 2024 at 9:07 PM •Isaac Rv  wrote:

> Ok entiendo sí, pero mi versión sigue en la 13.12 y necesito que sea
> 13.14, me indica que ya no tiene actualizaciones pero realmente sí, ya no
> sé cómo actualizarla a la 13.14
>

Hi

Please make sure that your postgres repository is set properly, that's the
only reason that it's not finding V13.14. Please follow the link below.

https://www.postgresql.org/download/linux/redhat/

There is another way to avoid it by downloading the V13.14 on your system
and then install this version on your system which will upgrade your
existing installation.

Regards
Kashif Zeeshan
Bitnine Global

>
> El sáb, 27 abr 2024 a las 9:29, Kashif Zeeshan ()
> escribió:
>
>> Glad to be of help.
>> pg_uprade is used with major version upgrade e.g. from PG13 to 14 etc
>>
>> Regards
>> Kashif Zeeshan
>> Bitnine Global
>>
>> On Fri, Apr 26, 2024 at 10:47 PM •Isaac Rv 
>> wrote:
>>
>>> Hola, lo acabo de hacer, quedó bien luego detuve el servidor, aplique
>>> otra vez el   sudo yum update postgresql13 y me devolvió otra vez el
>>> mensaje que ya no tiene más actualizaciones pendientes
>>> Veo que esta el pg_upgrade, pero no entiendo bien cómo usarlo
>>>
>>> Saludos y muchas gracias
>>>
>>> El vie, 26 abr 2024 a las 11:34, Kashif Zeeshan (<
>>> kashi.zees...@gmail.com>) escribió:
>>>


 On Fri, Apr 26, 2024 at 9:22 PM •Isaac Rv 
 wrote:

> Mira intente con el yum y si actualizó pero sin embargo no actualizo a
> la 13.14
>
> sudo yum update postgresql13
> Updating Subscription Management repositories.
>
> This system is registered with an entitlement server, but is not
> receiving updates. You can use subscription-manager to assign 
> subscriptions.
>
> Last metadata expiration check: 0:07:02 ago on Fri 26 Apr 2024
> 10:01:36 AM CST.
> Dependencies resolved.
> Nothing to do.
> Complete!
>

 It seemed yum is not able to get the latest package update, try
 clearing the cache and rebuilding it

 yum clean all

 yum makecache



>
> El jue, 25 abr 2024 a las 23:16, Kashif Zeeshan (<
> kashi.zees...@gmail.com>) escribió:
>
>>
>>
>> On Thu, Apr 25, 2024 at 11:55 PM •Isaac Rv 
>> wrote:
>>
>>> Entiendo si, me han dicho que es sencillo, pero no entiendo si solo
>>> descargo los binarios y en cual carpeta reemplazo? no hay una guía cómo 
>>> tal
>>> de cómo realizarlo, me  podrías ayudar?
>>>
>>
>> Follow the below steps
>> 1. Backup your data
>> 2. Review the release notes of the update release
>> 3. Stop the PG Server
>> 4. Upgrade postgres to newer version, e.g. on CentOS use the command
>> 'sudo yum update postgresql'
>> 5. Restart PG Server
>>
>> Thanks
>> Kashif Zeeshan
>> Bitnine Global
>>
>>>
>>> El jue, 25 abr 2024 a las 11:20, Kashif Zeeshan (<
>>> kashi.zees...@gmail.com>) escribió:
>>>
 Hi Isaac

 You are doing the minor version upgrade so it's not a big effort as
 compared to major version upgrade, following is the process to do it.

 *Minor releases never change the internal storage format and are
 always compatible with earlier and later minor releases of the same 
 major
 version number. For example, version 10.1 is compatible with version 
 10.0
 and version 10.6. Similarly, for example, 9.5.3 is compatible with 
 9.5.0,
 9.5.1, and 9.5.6. To update between compatible versions, you simply 
 replace
 the executables while the server is down and restart the server. The 
 data
 directory remains unchanged — minor upgrades are that simple.*


 Please follow the links below for more information.
 https://www.postgresql.org/docs/13/upgrading.html
 https://www.postgresql.org/support/versioning/

 Thanks
 Kashif Zeeshan
 Bitnine Global

 On Thu, Apr 25, 2024 at 9:37 PM •Isaac Rv 
 wrote:

> Hello everyone, I hope you're doing well. Does anyone have a guide
> or know how to perform an upgrade from PostgreSQL 13.12 to 13.14 on 
> Linux?
> I've searched in various places but haven't found any solid guides, 
> and
> truth be told, I'm a bit of a novice with PostgreSQL. Any help would 
> be
> appreciated.
>

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[David G. Johnston]

On Monday, April 29, 2024, Tom Lane  wrote:

> Daniel Gustafsson  writes:
> >> On 28 Apr 2024, at 20:52, Tom Lane  wrote:
>
>
> >> This is of course not bulletproof: with a sufficiently weird
> >> bootstrap superuser name, we could get false matches to parts
> >> of "regress_dump_test_role" or to privilege strings.  That
> >> seems unlikely enough to live with, but I wonder if anybody has
> >> a better idea.
>
> > I think that will be bulletproof enough to keep it working in the
> buildfarm and
> > among 99% of hackers.
>
> It occurred to me to use "aclexplode" to expand the initprivs, and
> then we can substitute names with simple equality tests.  The test
> query is a bit more complicated, but I feel better about it.
>

My solution to this was to rely on the fact that the bootstrap superuser is
assigned OID 10 regardless of its name.

David J.

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

"David G. Johnston"  writes:
> My solution to this was to rely on the fact that the bootstrap superuser is
> assigned OID 10 regardless of its name.

Yeah, I wrote it that way to start with too, but reconsidered
because

(1) I don't like hard-coding numeric OIDs.  We can avoid that in C
code but it's harder to do in SQL.

(2) It's not clear to me that this test couldn't be run by a
non-bootstrap superuser.  I think "current_user" is actually
the correct thing for the role executing the test.

regards, tom lane

Re: [PATCH] Fix bug when calling strncmp in check_authmethod_valid

[Richard Guo]

On Tue, Apr 30, 2024 at 10:41 AM Jingxian Li  wrote:

> Attached is a patch that fixes bug when calling strncmp function, in
> which case the third argument (authmethod - strchr(authmethod, ' '))
> may be negative, which is not as expected..


Nice catch.  I think you're right from a quick glance.

Thanks
Richard

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[David G. Johnston]

On Monday, April 29, 2024, Tom Lane  wrote:

> "David G. Johnston"  writes:
> > My solution to this was to rely on the fact that the bootstrap superuser
> is
> > assigned OID 10 regardless of its name.
>
> Yeah, I wrote it that way to start with too, but reconsidered
> because
>
> (1) I don't like hard-coding numeric OIDs.  We can avoid that in C
> code but it's harder to do in SQL.


If the tests don’t involve, e.g., the predefined role pg_monitor and its
grantor of the memberships in the other predefined roles, this indeed can
be avoided.  So I think my test still needs to check for 10 even if some
other superuser is allowed to produce the test output since a key output in
my case was the bootstrap superuser and the initdb roles.


> (2) It's not clear to me that this test couldn't be run by a
> non-bootstrap superuser.  I think "current_user" is actually
> the correct thing for the role executing the test.
>

Agreed, testing against current_role is correct if the things being queried
were created while executing the test.  I would need to do this as well to
remove the current requirement that my tests be run by the bootstrap
superuser.

David J.

Re: Removing unneeded self joins

[Alexander Lakhin]

Hello Alexander,

23.10.2023 12:47, Alexander Korotkov wrote:

I think this patch makes substantial improvement to query planning.
It has received plenty of reviews.  The code is currently in quite
good shape.  I didn't manage to find the cases when this optimization
causes significant overhead to planning time.  Even if such cases will
be spotted there is a GUC option to disable this feature.  So, I'll
push this if there are no objections.


I've discovered another failure, introduced by d3d55ce57.
Please try the following:
CREATE TABLE t (a int unique, b float);
SELECT * FROM t NATURAL JOIN LATERAL
 (SELECT * FROM t t2 TABLESAMPLE SYSTEM (t.b)) t2;

With asserts enabled, it triggers
TRAP: failed Assert("!bms_is_member(rti, lateral_relids)"), File: 
"initsplan.c", Line: 697, PID: 3074054
ExceptionalCondition at assert.c:52:13
create_lateral_join_info at initsplan.c:700:8
query_planner at planmain.c:257:2
grouping_planner at planner.c:1523:17
subquery_planner at planner.c:1098:2
standard_planner at planner.c:415:9
planner at planner.c:282:12
pg_plan_query at postgres.c:904:9
pg_plan_queries at postgres.c:996:11
exec_simple_query at postgres.c:1193:19
PostgresMain at postgres.c:4684:27

With no asserts, I get:
ERROR:  failed to construct the join relation

Please take a look at this.

Best regards,
Alexander

Re: tablecmds.c/MergeAttributes() cleanup

[Ashutosh Bapat]

On Mon, Apr 29, 2024 at 6:46 PM Robert Haas  wrote:

> On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat
>  wrote:
> > Yes please. Probably this issue surfaced again after we reverted
> compression and storage fix? Please  If that's the case, please add it to
> the open items.
>
> This is still on the open items list and I'm not clear who, if anyone,
> is working on fixing it.
>
> It would be good if someone fixed it. :-)
>

Here's a patch fixing it.

I have added the reproducer provided by Alexander as a test. I thought of
improving that test further to test the compression of the inherited table
but did not implement it since we haven't documented the behaviour of
compression with inheritance. Defining and implementing compression
behaviour for inherited tables was the goal
of 0413a556990ba628a3de8a0b58be020fd9a14ed0, which has been reverted.

-- 
Best Wishes,
Ashutosh Bapat
From 7c1ff7b17933eef9523486a2c0a054836db9cf24 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Tue, 30 Apr 2024 11:19:43 +0530
Subject: [PATCH] Fix segmentation fault in MergeInheritedAttribute()

While converting a pg_attribute tuple into a ColumnDef, ColumnDef::compression
remains NULL if there is no compression method set fot the attribute. Calling
strcmp() with NULL ColumnDef::compression, when comparing compression methods
of parents, causes segmentation fault in MergeInheritedAttribute(). Skip
comparing compression methods if either of them is NULL.

Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/b22a6834-aacb-7b18-0424-a3f5fe889667%40gmail.com
---
 src/backend/commands/tablecmds.c  | 16 ++--
 src/test/regress/expected/compression.out | 10 +++---
 src/test/regress/sql/compression.sql  |  8 +---
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..e29f96e357 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3430,12 +3430,16 @@ MergeInheritedAttribute(List *inh_columns,
 	 */
 	if (prevdef->compression == NULL)
 		prevdef->compression = newdef->compression;
-	else if (strcmp(prevdef->compression, newdef->compression) != 0)
-		ereport(ERROR,
-(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("column \"%s\" has a compression method conflict",
-		attributeName),
- errdetail("%s versus %s", prevdef->compression, newdef->compression)));
+	else if (newdef->compression != NULL)
+	{
+		if (strcmp(prevdef->compression, newdef->compression) != 0)
+			ereport(ERROR,
+	(errcode(ERRCODE_DATATYPE_MISMATCH),
+	 errmsg("column \"%s\" has a compression method conflict",
+			attributeName),
+	 errdetail("%s versus %s",
+			   prevdef->compression, newdef->compression)));
+	}
 
 	/*
 	 * Check for GENERATED conflicts
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 834b7555cb..4dd9ee7200 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -223,15 +223,18 @@ SELECT pg_column_compression(f1) FROM cmpart2;
  pglz
 (1 row)
 
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
+-- test compression with inheritance
+CREATE TABLE cminh() INHERITS(cmdata, cmdata1); -- error
 NOTICE:  merging multiple inherited definitions of column "f1"
 ERROR:  column "f1" has a compression method conflict
 DETAIL:  pglz versus lz4
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
+CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata); -- error
 NOTICE:  merging column "f1" with inherited definition
 ERROR:  column "f1" has a compression method conflict
 DETAIL:  pglz versus lz4
+CREATE TABLE cmdata3(f1 text);
+CREATE TABLE cminh() INHERITS (cmdata, cmdata3);
+NOTICE:  merging multiple inherited definitions of column "f1"
 -- test default_toast_compression GUC
 SET default_toast_compression = '';
 ERROR:  invalid value for parameter "default_toast_compression": ""
@@ -251,6 +254,7 @@ INSERT INTO cmdata VALUES (repeat('123456789', 4004));
  f1 | text |   |  | | extended | lz4 |  | 
 Indexes:
 "idx" btree (f1)
+Child tables: cminh
 
 SELECT pg_column_compression(f1) FROM cmdata;
  pg_column_compression 
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 7179a5002e..490595fcfb 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -93,9 +93,11 @@ INSERT INTO cmpart VALUES (repeat('123456789', 4004));
 SELECT pg_column_compression(f1) FROM cmpart1;
 SELECT pg_column_compression(f1) FROM cmpart2;
 
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
+-- test compression with inheritance
+CREATE TABLE cminh() INHERITS(cmdata, cmdata1); -- error
+

Re: Fix parallel vacuum buffer usage reporting

[Anthonin Bonnefoy]

I've done some additional tests to validate the reported numbers. Using
pg_statio, it's possible to get the minimum number of block hits (Full
script attached).

-- Save block hits before vacuum
SELECT pg_stat_force_next_flush();
SELECT heap_blks_hit, idx_blks_hit FROM pg_statio_all_tables where
relname='vestat' \gset
vacuum (verbose, index_cleanup on) vestat;
-- Check the difference
SELECT pg_stat_force_next_flush();
SELECT heap_blks_hit - :heap_blks_hit as delta_heap_hit,
   idx_blks_hit - :idx_blks_hit as delta_idx_hit,
   heap_blks_hit - :heap_blks_hit + idx_blks_hit - :idx_blks_hit as sum
FROM pg_statio_all_tables where relname='vestat';

Output:
...
buffer usage: 14676 hits, 0 misses, 667 dirtied
buffer usage (new): 16081 hits, 0 misses, 667 dirtied
...
 -[ RECORD 1 ]--+--
delta_heap_hit | 9747
delta_idx_hit  | 6325
sum| 16072

>From pg_statio, we had 16072 blocks for the relation + indexes.
Pre-patch, we are under reporting with 14676.
Post-patch, we have 16081. The 9 additional block hits come from vacuum
accessing catalog tables like pg_class or pg_class_oid_index.


vacuum_block_with_pgstatio.sql
Description: Binary data

Re: speed up a logical replica setup

[Amit Kapila]

On Mon, Apr 29, 2024 at 5:28 PM Amit Kapila  wrote:
>
> On Mon, Apr 29, 2024 at 5:23 PM Euler Taveira  wrote:
>

I was trying to test this utility when 'sync_replication_slots' is on
and it gets in an ERROR loop [1] and never finishes. Please find the
postgresql.auto used on the standby attached. I think if the standby
has enabled sync_slots, you need to pass dbname in
GenerateRecoveryConfig(). I couldn't test it further but I wonder if
there are already synced slots on the standby (either due to
'sync_replication_slots' or users have used
pg_sync_replication_slots() before invoking pg_createsubscriber),
those would be retained as it is on new subscriber and lead to
unnecessary WAL retention and dead rows.

[1]
2024-04-30 11:50:43.239 IST [12536] LOG:  slot sync worker started
2024-04-30 11:50:43.247 IST [12536] ERROR:  slot synchronization
requires dbname to be specified in primary_conninfo

-- 
With Regards,
Amit Kapila.


postgresql.auto.standby.conf
Description: Binary data

Re: Extension Enhancement: Buffer Invalidation in pg_buffercache

[Maksim Milyutin]

On 29.04.2024 23:59, Thomas Munro wrote:

On Tue, Apr 30, 2024 at 7:17 AM Thomas Munro  wrote:

On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin  wrote:

Should not we call at the end the StrategyFreeBuffer() function to add target 
buffer to freelist and not miss it after invalidation?

Please take a look at this issue, current implementation of 
EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently 
and will not be reused again

I don't think that's true: it is not lost permanently, it'll be found
by the regular clock hand.  Perhaps it should be put on the freelist
so it can be found again quickly, but I'm not sure that's a bug, is
it?



Yeah, you are right. Thanks for clarification.

CLOCK algorithm will reuse it eventually but being of evicted cleared 
buffer in freelist might greatly restrict the time of buffer allocation 
when all others buffers were in use.


--
Best regards,
Maksim Milyutin