Re: Add common function ReplicationOriginName.

[Amit Kapila]

On Mon, Oct 10, 2022 at 7:09 PM Amit Kapila  wrote:
>
> On Tue, Sep 27, 2022 at 5:04 PM Aleksander Alekseev
>  wrote:
> >
> > Hi Peter,
> >
> > > PSA patch v3 to combine the different replication origin name
> > > formatting in a single function ReplicationOriginNameForLogicalRep as
> > > suggested.
> >
> > LGTM except for minor issues with the formatting; fixed.
> >
>
> LGTM as well. I'll push this tomorrow unless there are any more comments.
>

Pushed.

-- 
With Regards,
Amit Kapila.

Re: Eliminating SPI from RI triggers - take 2

[Amit Langote]

On Fri, Oct 7, 2022 at 7:17 PM Amit Langote  wrote:
> On Fri, Oct 7, 2022 at 19:15 Alvaro Herrera  wrote:
>> On 2022-Oct-07, Amit Langote wrote:
>> > > Thanks for the heads up.  Hmm, this I am not sure how to reproduce on
>> > > my own, so I am currently left with second-guessing what may be going
>> > > wrong on 32 bit machines with whichever of the 4 patches.
>> > >
>> > > For now, I'll just post 0001, which I am claiming has no semantic
>> > > changes (proof pending), to rule out that that one's responsible.
>> >
>> > Nope, not 0001.  Here's 0001+0002.

I had forgotten to actually attach anything with that email.

>> Please note that you can set up a github repository so that cirrus-ci
>> tests whatever patches you like, without having to post them to
>> pg-hackers.  See src/tools/ci/README, it takes three minutes if you
>> already have the account and repository.
>
> Ah, that’s right.  Will do so, thanks for the suggestion.

I'm waiting to hear from GitHub Support to resolve an error I'm facing
trying to add Cirrus CI to my account.


--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
From 363e7539afea9b5ef287865b5176395818e880df Mon Sep 17 00:00:00 2001
From: amitlan 
Date: Tue, 28 Jun 2022 17:15:51 +0900
Subject: [PATCH v6 1/4] Avoid using SPI in RI trigger functions

Currently, ri_PlanCheck() uses SPI_prepare() to get an "SPI plan"
containing a CachedPlanSource for the SQL query that a given RI
trigger function uses to implement an RI check.  Furthermore,
ri_PerformCheck() calls SPI_execute_snapshot() on the "SPI plan"
to execute the query for a given snapshot.

This commit invents ri_PlanCreate() and ri_PlanExecute() to take
the place of SPI_prepare() and SPI_execute_snapshot(), respectively.

ri_PlanCreate() will create an "RI plan" for a given query, using a
caller-specified (caller of ri_PlanCheck() that is) callback
function.  For example, the callback ri_SqlStringPlanCreate() will
produce a CachedPlanSource for the input SQL string, just as
SPI_prepare() would.

ri_PlanExecute() will execute the "RI plan" by calling a
caller-specific callback function whose pointer is saved within the
"RI Plan" data structure (struct RIPlan).  For example, the callback
ri_SqlStringPlanExecute() will fetch a CachedPlan for given
CachedPlanSource found in the "RI plan" and execute its PlannedStmt
by invoking the executor, just as SPI_execute_snapshot() would.
Details such as which snapshot to use are now fully controlled by
ri_PerformCheck(), whereas the previous arrangement relied on the
SPI logic for snapshot management.

ri_PlanCreate(), ri_PlanExecute(), and the "RI plan" data structure
they manipulate are pluggable such that it will be possible for the
future commits to replace the current SQL string based implementation
of some RI checks with something as simple as a C function to directly
scan the underlying table/index of the referencing or the referenced
table.

NB: RI_Initial_Check() and RI_PartitionRemove_Check() still use the
the SPI_prepare()/SPI_execute_snapshot() combination, because I
haven't yet added a proper DestReceiver in ri_SqlStringPlanExecute()
to receive and process the tuples that the execution would produce,
which those RI_* functions will need.
---
 src/backend/executor/spi.c  |   2 +-
 src/backend/utils/adt/ri_triggers.c | 600 +++-
 2 files changed, 490 insertions(+), 112 deletions(-)

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index fd5796f1b9..a30553ea67 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -762,7 +762,7 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
  * end of the command.
  *
  * This is currently not documented in spi.sgml because it is only intended
- * for use by RI triggers.
+ * for use by some functions in ri_triggers.c.
  *
  * Passing snapshot == InvalidSnapshot will select the normal behavior of
  * fetching a new snapshot for each query.
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 1d503e7e01..cfebd9c4f2 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -9,7 +9,7 @@
  *	across query and transaction boundaries, in fact they live as long as
  *	the backend does.  This works because the hashtable structures
  *	themselves are allocated by dynahash.c in its permanent DynaHashCxt,
- *	and the SPI plans they point to are saved using SPI_keepplan().
+ *	and the CachedPlanSources they point to are saved in CachedMemoryContext.
  *	There is not currently any provision for throwing away a no-longer-needed
  *	plan --- consider improving this someday.
  *
@@ -40,6 +40,8 @@
 #include "parser/parse_coerce.h"
 #include "parser/parse_relation.h"
 #include "storage/bufmgr.h"
+#include "tcop/pquery.h"
+#include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
@@ -127,10 +129,55 @@ typedef struct RI_ConstraintInfo
 	dlist_no

Re: future of serial and identity columns

[Peter Eisentraut]

On 04.10.22 09:41, Peter Eisentraut wrote:
Attached is a demo patch how the implementation of this change would 
look like.  This creates a bunch of regression test failures, but 
AFAICT, those are mainly display differences and some very peculiar test 
setups that are intentionally examining some edge cases.  These would 
need to be investigated in more detail, of course.


The feedback was pretty positive, so I dug through all the tests to at 
least get to the point where I could see the end of it.  The attached 
patch 0001 is the actual code and documentation changes.  The 0002 patch 
is just tests randomly updated or disabled to make the whole suite pass. 
 This reveals that there are a few things that would warrant further 
investigation, in particular around extensions and partitioning.  To be 
continued.
From c3c322c277e58205b7074bbc504f94029f98a0bd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 11 Oct 2022 08:24:34 +0200
Subject: [PATCH v2 1/2] WIP: Change serial types to map to identity columns

This changes serial types to convert internally to identity columns,
instead of the custom construction involving nextval defaults that
they previously did.
---
 contrib/test_decoding/expected/ddl.out| 50 ++---
 doc/src/sgml/datatype.sgml| 37 +++--
 doc/src/sgml/extend.sgml  | 10 +--
 doc/src/sgml/func.sgml| 12 +--
 src/backend/parser/parse_utilcmd.c| 75 +++
 src/test/regress/expected/publication.out | 30 
 .../regress/expected/replica_identity.out | 30 
 7 files changed, 80 insertions(+), 164 deletions(-)

diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 9a28b5ddc5aa..9363d140d2af 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -484,12 +484,12 @@ CREATE TABLE replication_metadata (
 WITH (user_catalog_table = true)
 ;
 \d+ replication_metadata
- Table 
"public.replication_metadata"
-  Column  |  Type   | Collation | Nullable | Default   
   | Storage  | Stats target | Description 
---+-+---+--+--+--+--+-
- id   | integer |   | not null | 
nextval('replication_metadata_id_seq'::regclass) | plain|  | 
- relation | name|   | not null |   
   | plain|  | 
- options  | text[]  |   |  |   
   | extended |  | 
+ Table "public.replication_metadata"
+  Column  |  Type   | Collation | Nullable | Default  
| Storage  | Stats target | Description 
+--+-+---+--+--+--+--+-
+ id   | integer |   | not null | generated by default as identity 
| plain|  | 
+ relation | name|   | not null |  
| plain|  | 
+ options  | text[]  |   |  |  
| extended |  | 
 Indexes:
 "replication_metadata_pkey" PRIMARY KEY, btree (id)
 Options: user_catalog_table=true
@@ -498,12 +498,12 @@ INSERT INTO replication_metadata(relation, options)
 VALUES ('foo', ARRAY['a', 'b']);
 ALTER TABLE replication_metadata RESET (user_catalog_table);
 \d+ replication_metadata
- Table 
"public.replication_metadata"
-  Column  |  Type   | Collation | Nullable | Default   
   | Storage  | Stats target | Description 
---+-+---+--+--+--+--+-
- id   | integer |   | not null | 
nextval('replication_metadata_id_seq'::regclass) | plain|  | 
- relation | name|   | not null |   
   | plain|  | 
- options  | text[]  |   |  |   
   | extended |  | 
+ Table "public.replication_metadata"
+  Column  |  Type   | Collation | Nullable | Default  
| Storage  | Stats target | Description 
+--+-+---+--+--+--+--+-
+ id   | integer |   | not null | generated by default as identity 
| plain|  | 
+ relation | name|   | not null |  
| plain|  | 
+ options  | text[]  

Re: Add common function ReplicationOriginName.

[Aleksander Alekseev]

Hi Amit,

> Pushed.

Thanks!

> I don't have a strong opinion on whether we should be really worried
> by this. But in case we do, here is the patch.

This leaves us one patch to deal with.

-- 
Best regards,
Aleksander Alekseev


v4-0001-Pass-Size-size_t-as-a-2nd-argument-of-snprintf.patch
Description: Binary data

Re: START_REPLICATION SLOT causing a crash in an assert build

[Masahiko Sawada]

On Sun, Oct 9, 2022 at 2:42 AM Andres Freund  wrote:
>
> On 2022-10-08 09:53:50 -0700, Andres Freund wrote:
> > On 2022-10-07 19:56:33 -0700, Andres Freund wrote:
> > > I'm planning to push this either later tonight (if I feel up to it after
> > > cooking dinner) or tomorrow morning PST, due to the release wrap deadline.
> >
> > I looked this over again, tested a bit more, and pushed the adjusted 15 and
> > master versions to github to get a CI run. Once that passes, as I expect, 
> > I'll
> > push them for real.
>
> Those passed and thus pushed.
>
> Thanks for the report, debugging and review everyone!

Thanks!

>
>
> I think we need at least the following tests for replslots:
> - a reset while decoding is ongoing works correctly
> - replslot stats continue to be accumulated after stats have been removed
>
>
> I wonder how much it'd take to teach isolationtester to handle the replication
> protocol...

I think we can do these tests by using pg_recvlogical in TAP tests.
I've attached a patch to do that.

Regards,

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


regression_test_for_replslot_stats.patch
Description: Binary data

Re: problems with making relfilenodes 56-bits

[Dilip Kumar]

On Mon, Oct 10, 2022 at 5:40 PM Robert Haas  wrote:
>
> On Mon, Oct 10, 2022 at 5:16 AM Dilip Kumar  wrote:
> > I have also executed my original test after applying these patches on
> > top of the 56 bit relfilenode patch.  So earlier we saw the WAL size
> > increased by 11% (66199.09375 kB to 73906.984375 kB) and after this
> > patch now the WAL generated is 58179.2265625.  That means in this
> > particular example this patch is reducing the WAL size by 12% even
> > with the 56 bit relfilenode patch.
>
> That's a very promising result, but the question in my mind is how
> much work would be required to bring this patch to a committable
> state?

Right, the results are promising.  I have done some more testing with
make installcheck WAL size (fpw=off) and I have seen a similar gain
with this patch.

1. Head: 272 MB
2. 56 bit RelfileLocator: 285 MB
3. 56 bit RelfileLocator + this patch: 261 MB

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

Re: ps command does not show walsender's connected db

[bt22nakamorit]

2022-10-10 16:12 Bharath Rupireddy wrote:

Thanks. LGTM.

Thank you for your review!
I have this issue posted on Commitfest 2022-11 with title "show 
walsender's connected db for ps command entry".

May I change the status to "Ready for Committer"?

Best,
Tatsuhiro Nakamori

Re: Non-decimal integer literals

[Peter Eisentraut]

On 11.10.22 05:29, Junwang Zhao wrote:

What do you think if we move these code into a static inline function? like:

static inline char*
process_digits(char *ptr, int32 *result)
{
...
}


How would you handle the different ways each branch checks for valid 
digits and computes the value of each digit?

Re: list of acknowledgments for PG15

[Peter Eisentraut]

On 10.10.22 08:41, Tom Lane wrote:

What remains is to
define when is the cutoff point between "acknowledge in v15" versus
"acknowledge in v16".  I don't have a strong opinion about that,
but I'd like it to be more than 24 hours before the 15.0 wrap.
Could we make the cutoff be, say, beta1?


beta1 is too early, because a significant portion of the names comes in 
after beta1.  rc1 would be ok, I think.

Is there any plan to support online schem change in postgresql?

[jiye]

Hi,


As we know postgres using high level lock when do alter table or other ddl 
commands,
It will block any dml operation, while it also will block by long term dml 
operation.


Like what discuss as follow :
https://dba.stackexchange.com/questions/293992/make-alter-table-wait-for-lock-without-blocking-anything-else.


I know that postgres try to avoid rewrite table when alter table happen , and 
so far, it support serveral ddl using concurrently feature,
Like create indexes. But like alter table add/drop colum, alter column type, it 
also will trigger rewrtie table . Long term block will make application offline 
in long times.


So is there any plan to support these ddl online and lock free？if not could you 
explain the  technological difficulty ？


Thanks and wating your respond!

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Richard Guo]

On Tue, Oct 11, 2022 at 1:44 PM Kyotaro Horiguchi 
wrote:

> At Mon, 10 Oct 2022 08:53:55 +0530, Bharath Rupireddy <
> bharath.rupireddyforpostg...@gmail.com> wrote in
> > It looks like we have an unnecessary XLogSegNoOffsetToRecPtr() in
> > XLogReaderValidatePageHeader(). We pass the start LSN of the WAL page
> > and check if it matches with the LSN that was stored in the WAL page
> > header (xlp_pageaddr). We find segno, offset and LSN again using
> > XLogSegNoOffsetToRecPtr(). This happens to be the same as the passed
> > in LSN 'recptr'.
>
> Yeah, that's obviously useless. It looks like a thinko in pg93 when
> recptr became to be directly passed from the caller instead of
> calculating from static variables for file, segment and in-segment
> offset.


+1. This should be introduced in 7fcbf6a4 as a thinko. A grep search
shows other callers of XLogSegNoOffsetToRecPtr have no such issue.

Thanks
Richard

Re: Is there any plan to support online schem change in postgresql?

[hubert depesz lubaczewski]

On Tue, Oct 11, 2022 at 05:43:03PM +0800, jiye wrote:
> As we know postgres using high level lock when do alter table or other ddl 
> commands,
> It will block any dml operation, while it also will block by long term dml 
> operation.

Most of the things can be already done in non-blocking (or almost
non-blocking way) if you just do it in a way that takes concurrency into
account.

Specifically - I have no problem adding/deleting columns.

Best regards,

depesz

Re: Non-decimal integer literals

[Junwang Zhao]

On Tue, Oct 11, 2022 at 4:59 PM Peter Eisentraut
 wrote:
>
> On 11.10.22 05:29, Junwang Zhao wrote:
> > What do you think if we move these code into a static inline function? like:
> >
> > static inline char*
> > process_digits(char *ptr, int32 *result)
> > {
> > ...
> > }
>
> How would you handle the different ways each branch checks for valid
> digits and computes the value of each digit?
>

Didn't notice that, sorry for the noise ;(


-- 
Regards
Junwang Zhao

Re: create subscription - improved warning message

[Alvaro Herrera]

On 2022-Oct-10, Peter Smith wrote:

> On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila  wrote:

> > I think the below gives accurate information:
> > WARNING: subscription was created, but is not connected
> > HINT: You should create the slot manually, enable the subscription,
> > and run %s to initiate replication.

I guess this is reasonable, but how do I know what slot name do I have
to create?  Maybe it'd be better to be explicit about that:

HINT: You should create slot \"%s\" manually, enable the subscription, and run 
%s to initiate replication.

though this still leaves a lot unexplained about that slot creation
(which options do they have to use).


If this sounds like too much for a HINT, perhaps we need a documentation
subsection that explains exactly what to do, and have this HINT
reference the documentation?  I don't think we do that anywhere else,
though.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."  (E. Dijkstra)

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Bharath Rupireddy]

On Tue, Oct 11, 2022 at 3:19 PM Richard Guo  wrote:
>
>
> On Tue, Oct 11, 2022 at 1:44 PM Kyotaro Horiguchi  
> wrote:
>>
>> At Mon, 10 Oct 2022 08:53:55 +0530, Bharath Rupireddy 
>>  wrote in
>> > It looks like we have an unnecessary XLogSegNoOffsetToRecPtr() in
>> > XLogReaderValidatePageHeader(). We pass the start LSN of the WAL page
>> > and check if it matches with the LSN that was stored in the WAL page
>> > header (xlp_pageaddr). We find segno, offset and LSN again using
>> > XLogSegNoOffsetToRecPtr(). This happens to be the same as the passed
>> > in LSN 'recptr'.
>>
>> Yeah, that's obviously useless. It looks like a thinko in pg93 when
>> recptr became to be directly passed from the caller instead of
>> calculating from static variables for file, segment and in-segment
>> offset.
>
>
> +1. This should be introduced in 7fcbf6a4 as a thinko. A grep search
> shows other callers of XLogSegNoOffsetToRecPtr have no such issue.

Thanks for reviewing. It's a pretty-old code that exists in 9.5 or
earlier [1], definitely not introduced by 7fcbf6a4.

[1] see XLogReaderValidatePageHeader() in
https://github.com/BRupireddy/postgres/blob/REL9_5_STABLE/src/backend/access/transam/xlogreader.c

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

Re: ps command does not show walsender's connected db

[Bharath Rupireddy]

On Tue, Oct 11, 2022 at 2:11 PM bt22nakamorit
 wrote:
>
> 2022-10-10 16:12 Bharath Rupireddy wrote:
> > Thanks. LGTM.
> Thank you for your review!
> I have this issue posted on Commitfest 2022-11 with title "show
> walsender's connected db for ps command entry".
> May I change the status to "Ready for Committer"?

Done - https://commitfest.postgresql.org/40/3937/.

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

Re: Allow tests to pass in OpenSSL FIPS mode

[Peter Eisentraut]

On 04.10.22 17:45, Peter Eisentraut wrote:
While working on the column encryption patch, I wanted to check that 
what is implemented also works in OpenSSL FIPS mode.  I tried running 
the normal test suites after switching the OpenSSL installation to FIPS 
mode, but that failed all over the place.  So I embarked on fixing that. 


Of course, there are some some tests where we do want to test MD5 
functionality, such as in the authentication tests or in the tests of 
the md5() function itself.  I think we can conditionalize these somehow. 


Let's make a small start on this.  The attached patch moves the tests of 
the md5() function to a separate test file.  That would ultimately make 
it easier to maintain a variant expected file for FIPS mode where that 
function will fail (similar to how we have done it for the pgcrypto tests).
From 78b6032444ca7db540a82ab72637c3571afbae82 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 11 Oct 2022 10:33:30 +0200
Subject: [PATCH] Put tests of md5() function into separate test file

In FIPS mode, these calls will fail.  By having them in a separate
file, it would make it easier to have an alternative output file or
selectively disable these tests.  This isn't done here; this is just
some preparation.
---
 src/test/regress/expected/md5.out | 91 +++
 src/test/regress/expected/strings.out | 88 --
 src/test/regress/parallel_schedule|  2 +-
 src/test/regress/sql/md5.sql  | 36 +++
 src/test/regress/sql/strings.sql  | 32 --
 5 files changed, 128 insertions(+), 121 deletions(-)
 create mode 100644 src/test/regress/expected/md5.out
 create mode 100644 src/test/regress/sql/md5.sql

diff --git a/src/test/regress/expected/md5.out 
b/src/test/regress/expected/md5.out
new file mode 100644
index ..c5dd801cef2d
--- /dev/null
+++ b/src/test/regress/expected/md5.out
@@ -0,0 +1,91 @@
+--
+-- MD5 test suite - from IETF RFC 1321
+-- (see: https://www.rfc-editor.org/rfc/rfc1321)
+--
+-- (The md5() function will error in OpenSSL FIPS mode.  By keeping
+-- this test in a separate file, it is easier to manage variant
+-- results.)
+select md5('') = 'd41d8cd98f00b204e9800998ecf8427e' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('a') = '0cc175b9c0f1b6a831c399e269772661' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('abc') = '900150983cd24fb0d6963f7d28e17f72' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('message digest') = 'f96b697d7cb7938d525a2f31aaf161d0' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('abcdefghijklmnopqrstuvwxyz') = 'c3fcd3d76192e4007dfb496cca67e13b' 
AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789') = 
'd174ab98d277d9f5a5611c2c9f419d9f' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select 
md5('12345678901234567890123456789012345678901234567890123456789012345678901234567890')
 = '57edf4a22be3c955ac49da2e2107b67a' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5(''::bytea) = 'd41d8cd98f00b204e9800998ecf8427e' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('a'::bytea) = '0cc175b9c0f1b6a831c399e269772661' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('abc'::bytea) = '900150983cd24fb0d6963f7d28e17f72' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('message digest'::bytea) = 'f96b697d7cb7938d525a2f31aaf161d0' AS 
"TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select md5('abcdefghijklmnopqrstuvwxyz'::bytea) = 
'c3fcd3d76192e4007dfb496cca67e13b' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select 
md5('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'::bytea) = 
'd174ab98d277d9f5a5611c2c9f419d9f' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
+select 
md5('12345678901234567890123456789012345678901234567890123456789012345678901234567890'::bytea)
 = '57edf4a22be3c955ac49da2e2107b67a' AS "TRUE";
+ TRUE 
+--
+ t
+(1 row)
+
diff --git a/src/test/regress/expected/strings.out 
b/src/test/regress/expected/strings.out
index 0f95b9400b69..69d7ed4ef1cf 100644
--- a/src/test/regress/expected/strings.out
+++ b/src/test/regress/expected/strings.out
@@ -2118,94 +2118,6 @@ select 
to_hex(256::bigint*256::bigint*256::bigint*256::bigint - 1) AS ""
  
 (1 row)
 
---
--- MD5 test suite - from IETF RFC 1321
--- (see: ftp://ftp.rfc-editor.org/in-notes/rfc1321.txt)
---
-select md5('') = 'd41d8cd98f00b204e9800998ecf8427e' AS "TRUE";
- TRUE 
---
- t
-(1 row)
-
-select md5('a') = '0cc175b9c0f1b6a831c399e269772661' AS "TRUE";
- TRUE 
---
- t
-(1 row)
-
-select md5('abc') = '900150983cd24fb0d6963f7d28e17f72' AS "TRUE";
- TRUE 
---
- t
-(1 row)
-
-select md5('message digest') = 'f96b697d7cb7938d525a2f31aaf161d0' AS "TRUE";
- TRUE 
---
- t
-(1 row)
-
-select md5('abcdefghijklmnopqrstuvwxyz') = 'c3fcd3d76192e4007dfb496cca67e13b' 
AS "TRUE";
- TRUE 
---
- t
-(1 row)
-
-select md5('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789') = 

Make EXPLAIN generate a generic plan for a parameterized query

[Laurenz Albe]

Today you get

test=> EXPLAIN SELECT * FROM tab WHERE col = $1;
ERROR:  there is no parameter $1

which makes sense.  Nonetheless, it would be great to get a generic plan
for such a query.  Sometimes you don't have the parameters (if you grab
the statement from "pg_stat_statements", or if it is from an error message
in the log, and you didn't enable "log_parameter_max_length_on_error").
Sometimes it is just very painful to substitute the 25 parameters from
the detail message.

With the attached patch you can get the following:

test=> SET plan_cache_mode = force_generic_plan;
SET
test=> EXPLAIN (COSTS OFF) SELECT * FROM pg_proc WHERE oid = $1;
  QUERY PLAN   
═══
 Index Scan using pg_proc_oid_index on pg_proc
   Index Cond: (oid = $1)
(2 rows)

That's not the same as a full-fledged EXPLAIN (ANALYZE, BUFFERS),
but it can definitely be helpful.

I tied that behavior to the setting of "plan_cache_mode" where you
are guaranteed to get a generic plan; I couldn't think of a better way.

Yours,
Laurenz Albe
From 2bc91581acd478d4648176b58745cadb835d5fbc Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 11 Oct 2022 13:05:31 +0200
Subject: [PATCH] Add EXPLAIN support for parameterized statements

If "plan_cache_mode = force_generic_plan", allow EXPLAIN to
generate generic plans for parameterized statements (that
have parameter placeholders like $1 in the statement text).

This repurposes hooks used by PL/pgSQL, so we better not try
to do that inside PL/pgSQL.
---
 doc/src/sgml/ref/explain.sgml | 10 +
 src/backend/parser/analyze.c  | 53 +++
 src/test/regress/expected/explain.out | 28 ++
 src/test/regress/sql/explain.sql  | 13 +++
 4 files changed, 104 insertions(+)

diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..928d67b9b4 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -321,6 +321,16 @@ ROLLBACK;
execution, and on machines that have relatively slow operating
system calls for obtaining the time of day.
   
+
+  
+   If  is set to
+   force_generic_plan, you can use EXPLAIN
+   to generate generic plans for statements that contain placeholders like
+   $1 without knowing the actual parameter type or value.
+   Note that expressions like $1 + $2 are ambiguous if you
+   don't specify the parameter data types, so you may have to add explicit type
+   casts in such cases.
+  
  
 
  
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6688c2a865..c481d45376 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -52,6 +52,7 @@
 #include "utils/guc.h"
 #include "utils/queryjumble.h"
 #include "utils/rel.h"
+#include "utils/plancache.h"
 #include "utils/syscache.h"
 
 
@@ -86,6 +87,10 @@ static Query *transformCallStmt(ParseState *pstate,
 CallStmt *stmt);
 static void transformLockingClause(ParseState *pstate, Query *qry,
    LockingClause *lc, bool pushedDown);
+static Node * fakeUnknownParam(ParseState *pstate, ParamRef *pref);
+static Node * coerceUnknownParam(ParseState *pstate, Param *param,
+ Oid targetTypeId, int32 targetTypeMod,
+ int location);
 #ifdef RAW_EXPRESSION_COVERAGE_TEST
 static bool test_raw_expression_coverage(Node *node, void *context);
 #endif
@@ -2895,6 +2900,22 @@ transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
 {
 	Query	   *result;
 
+	/*
+	 * If we EXPLAIN a statement and are certain to generate a generic plan,
+	 * we can tolerate undefined parameters.  For that purpose, supply
+	 * parameters of type "unknown" and coerce them to the appropriate type
+	 * as needed.
+	 * If we are called from PL/pgSQL, the hooks are already set for the
+	 * purpose of resolving variables, and we don't want to disturb that.
+	 */
+	if (plan_cache_mode == PLAN_CACHE_MODE_FORCE_GENERIC_PLAN &&
+		pstate->p_paramref_hook == NULL &&
+		pstate->p_coerce_param_hook == NULL)
+	{
+		pstate->p_paramref_hook = fakeUnknownParam;
+		pstate->p_coerce_param_hook = coerceUnknownParam;
+	}
+
 	/* transform contained query, allowing SELECT INTO */
 	stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
 
@@ -3466,6 +3487,38 @@ applyLockingClause(Query *qry, Index rtindex,
 	qry->rowMarks = lappend(qry->rowMarks, rc);
 }
 
+/*
+ * Return an "unknown" parameter for use with EXPLAIN of a parameterized
+ * statement.
+ */
+Node *
+fakeUnknownParam(ParseState *pstate, ParamRef *pref)
+{
+	Param  *param;
+
+	param = makeNode(Param);
+	param->paramkind = PARAM_EXTERN;
+	param->paramid = pref->number;
+	param->paramtype = UNKNOWNOID;
+	param->paramtypmod = -1;
+	param->paramcollid = InvalidOid;
+	param->location = pref->location;
+
+	return (Node *)param;
+}
+
+/*
+ * Set the parameter's type from "unknown" to the target type.
+ */
+Node *
+coerceUnknownParam(ParseState *pstate, Param *param, O

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

[Önder Kalacı]

Hi Kuroda Hayato,


> ===
> 01. relation.c - GetCheapestReplicaIdentityFullPath
>
> ```
>  * The reason that the planner would not pick partial indexes and
> indexes
>  * with only expressions based on the way currently
> baserestrictinfos are
>  * formed (e.g., col_1 = $1 ... AND col_N = $2).
> ```
>
> Is "col_N = $2" a typo? I think it should be "col_N = $N" or "attr1 = $1
> ... AND attrN = $N".
>
>
Yes, it is a typo, fixed now.


> ===
> 02. 032_subscribe_use_index.pl
>
> If a table has a primary key on the subscriber, it will be used even if
> enable_indexscan is false(legacy behavior).
> Should we test it?
>
>
Yes, good idea. I added two tests, one test that we cannot use regular
indexes when index scan is disabled, and another one that we use replica
identity index when index scan is disabled. This is useful to make sure if
someone changes the behavior can see the impact.


> ~~~
> 03. 032_subscribe_use_index.pl -  SUBSCRIPTION RE-CALCULATES INDEX AFTER
> CREATE/DROP INDEX
>
> I think this test seems to be not trivial, so could you write down the
> motivation?
>

makes sense, done


>
> ~~~
> 04. 032_subscribe_use_index.pl -  SUBSCRIPTION RE-CALCULATES INDEX AFTER
> CREATE/DROP INDEX
>
> ```
> # wait until the index is created
> $node_subscriber->poll_query_until(
> 'postgres', q{select count(*)=1 from pg_stat_all_indexes where
> indexrelname = 'test_replica_id_full_idx';}
> ) or die "Timed out while waiting for check subscriber tap_sub_rep_full_0
> updates one row via index";
> ```
>
> CREATE INDEX is a synchronous behavior, right? If so we don't have to wait
> here.
> ...And the comment in case of die may be wrong.
> (There are some cases like this)
>

It is not about CREATE INDEX being async. It is about pg_stat_all_indexes
being async. If we do not wait, the tests become flaky, because sometimes
the update has not been reflected in the view immediately.

This is explained here: PostgreSQL: Documentation: 14: 28.2. The Statistics
Collector 

*When using the statistics to monitor collected data, it is important to
> realize that the information does not update instantaneously. Each
> individual server process transmits new statistical counts to the collector
> just before going idle; so a query or transaction still in progress does
> not affect the displayed totals. Also, the collector itself emits a new
> report at most once per PGSTAT_STAT_INTERVAL milliseconds (500 ms unless
> altered while building the server). So the displayed information lags
> behind actual activity. However, current-query information collected by
> track_activities is always up-to-date.*
>



>
> ~~~
> 05. 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX UPDATEs MULTIPLE
> ROWS
>
> ```
> # Testcase start: SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS
> #
> # Basic test where the subscriber uses index
> # and touches 50 rows with UPDATE
> ```
>
> "touches 50 rows with UPDATE" -> "updates 50 rows", per other tests.
>
> fixed


> ~~~
> 06. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT
> USES AFTER ANALYZE
>
> I think this test seems to be not trivial, so could you write down the
> motivation?
> (Same as Re-calclate)
>

sure, done


>
> ~~~
> 07. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT
> USES AFTER ANALYZE
>
> ```
> # show that index_b is not used
> $node_subscriber->poll_query_until(
> 'postgres', q{select idx_scan=0 from pg_stat_all_indexes where
> indexrelname = 'index_b';}
> ) or die "Timed out while waiting for check subscriber tap_sub_rep_full
> updates two rows via index scan with index on high cardinality column-2";
> ```
>
> I think we don't have to wait here, is() should be used instead.
> poll_query_until() should be used only when idx_scan>0 is checked.
> (There are some cases like this)
>

Yes, makes sense


>
> ~~~
> 08. 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX ON PARTITIONED
> TABLES
>
> ```
> # make sure that the subscriber has the correct data
> $node_subscriber->poll_query_until(
> 'postgres', q{select sum(user_id+value_1+value_2)=550070 AND
> count(DISTINCT(user_id,value_1, value_2))=981 from users_table_part;}
> ) or die "ensure subscriber has the correct data at the end of the test";
> ```
>
>
Ah, for this case, we already have is() checks for the same results, this
is just a left-over from the earlier iterations


> I think we can replace it to wait_for_catchup() and is()...
> Moreover, we don't have to wait here because in above line we wait until
> the index is used on the subscriber.
> (There are some cases like this)
>

Fixed a few more such cases.

Thanks for the review! Attached v16.

Onder KALACI
From 1ebc170f821f290283aadd10c8c04480ad203580 Mon Sep 17 00:00:00 2001
From: Onder Kalaci 
Date: Tue, 17 May 2022 10:47:39 +0200
Subject: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full
 on the publisher

Using `REPLI

Re: Re: Is there any plan to support online schem change in postgresql?

[hubert depesz lubaczewski]

On Tue, Oct 11, 2022 at 08:31:53PM +0800, jiye wrote:
> But, as follow, if txn1 not commit (just like long term readonly txn), it 
> will block txn2's ddl job,  why alt add/drop column can not concurrently with 
> read only access?
> txn1： long term txn not commit access t1.
> txn2 waiting txn1 to commit or abort.
> txn3 wait txn2...

1. Please don't share code as screenshots.
2. If I understand your text above correctly, then the solution is
   trivial:
   
https://www.depesz.com/2019/09/26/how-to-run-short-alter-table-without-long-locking-concurrent-queries/

depesz

Re:Re: Is there any plan to support online schem change in postgresql?

[jiye]

But, as follow, if txn1 not commit (just like long term readonly txn), it will 
block txn2's ddl job,  why alt add/drop column can not concurrently with read 
only access?


txn1： long term txn not commit access t1.
txn2 waiting txn1 to commit or abort.

txn3 wait txn2...








At 2022-10-11 18:05:01, "hubert depesz lubaczewski"  wrote:
>On Tue, Oct 11, 2022 at 05:43:03PM +0800, jiye wrote:
>> As we know postgres using high level lock when do alter table or other ddl 
>> commands,
>> It will block any dml operation, while it also will block by long term dml 
>> operation.
>
>Most of the things can be already done in non-blocking (or almost
>non-blocking way) if you just do it in a way that takes concurrency into
>account.
>
>Specifically - I have no problem adding/deleting columns.
>
>Best regards,
>
>depesz

Re: Make EXPLAIN generate a generic plan for a parameterized query

[Tom Lane]

Laurenz Albe  writes:
> Today you get
> test=> EXPLAIN SELECT * FROM tab WHERE col = $1;
> ERROR:  there is no parameter $1
> which makes sense.  Nonetheless, it would be great to get a generic plan
> for such a query.

I can see the point, but it also seems like it risks masking stupid
mistakes.

> I tied that behavior to the setting of "plan_cache_mode" where you
> are guaranteed to get a generic plan; I couldn't think of a better way.

I think it might be better to drive it off an explicit EXPLAIN option,
perhaps

EXPLAIN (GENERIC_PLAN) SELECT * FROM tab WHERE col = $1;

This option (bikeshedding on the name welcome) would have the effect
both of allowing unanchored Param symbols and of temporarily forcing
generic-plan mode, so that you don't need additional commands to
set and reset plan_cache_mode.  We could also trivially add logic
to disallow the combination of ANALYZE and GENERIC_PLAN, which
would otherwise be a bit messy to prevent.

For context, it does already work to do this when you want to
investigate parameterized plans:

regression=# prepare foo as select * from tenk1 where unique1 = $1;
PREPARE
regression=# explain execute foo(42);
 QUERY PLAN  
-
 Index Scan using tenk1_unique1 on tenk1  (cost=0.29..8.30 rows=1 width=244)
   Index Cond: (unique1 = 42)
(2 rows)

If you're trying to investigate custom-plan behavior, then you
need to supply concrete parameter values somewhere, so I think
this approach is fine for that case.  (Shoehorning parameter
values into EXPLAIN options seems like it'd be a bit much.)
However, investigating generic-plan behavior this way is tedious,
since you have to invent irrelevant parameter values, plus mess
with plan_cache_mode or else run the explain half a dozen times.
So I can get behind having a more convenient way for that.

regards, tom lane

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Richard Guo]

On Tue, Oct 11, 2022 at 7:05 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Tue, Oct 11, 2022 at 3:19 PM Richard Guo 
> wrote:
> > On Tue, Oct 11, 2022 at 1:44 PM Kyotaro Horiguchi <
> horikyota@gmail.com> wrote:
> >> At Mon, 10 Oct 2022 08:53:55 +0530, Bharath Rupireddy <
> bharath.rupireddyforpostg...@gmail.com> wrote in
> >> > It looks like we have an unnecessary XLogSegNoOffsetToRecPtr() in
> >> > XLogReaderValidatePageHeader(). We pass the start LSN of the WAL page
> >> > and check if it matches with the LSN that was stored in the WAL page
> >> > header (xlp_pageaddr). We find segno, offset and LSN again using
> >> > XLogSegNoOffsetToRecPtr(). This happens to be the same as the passed
> >> > in LSN 'recptr'.
> >>
> >> Yeah, that's obviously useless. It looks like a thinko in pg93 when
> >> recptr became to be directly passed from the caller instead of
> >> calculating from static variables for file, segment and in-segment
> >> offset.
> >
> >
> > +1. This should be introduced in 7fcbf6a4 as a thinko. A grep search
> > shows other callers of XLogSegNoOffsetToRecPtr have no such issue.
>
> Thanks for reviewing. It's a pretty-old code that exists in 9.5 or
> earlier [1], definitely not introduced by 7fcbf6a4.
>
> [1] see XLogReaderValidatePageHeader() in
>
> https://github.com/BRupireddy/postgres/blob/REL9_5_STABLE/src/backend/access/transam/xlogreader.c


As I can see in 7fcbf6a4 ValidXLogPageHeader() is refactored as

-static bool
-ValidXLogPageHeader(XLogPageHeader hdr, int emode, bool segmentonly)
-{
-   XLogRecPtr  recaddr;
-
-   XLogSegNoOffsetToRecPtr(readSegNo, readOff, recaddr);

+static bool
+ValidXLogPageHeader(XLogReaderState *state, XLogRecPtr recptr,
+   XLogPageHeader hdr)
+{
+   XLogRecPtr  recaddr;
+   XLogSegNo   segno;
+   int32   offset;
+
+   Assert((recptr % XLOG_BLCKSZ) == 0);
+
+   XLByteToSeg(recptr, segno);
+   offset = recptr % XLogSegSize;
+
+   XLogSegNoOffsetToRecPtr(segno, offset, recaddr);

I think this is where the problem was introduced.

BTW, 7fcbf6a4 seems pretty old too as it can be found in 9.3 branch.

Thanks
Richard

Re: Add index scan progress to pg_stat_progress_vacuum

[Imseih (AWS), Sami]

>One idea would be to add a flag, say report_parallel_vacuum_progress,
>to IndexVacuumInfo struct and expect index AM to check and update the
>parallel index vacuum progress, say every 1GB blocks processed. The
>flag is true only when the leader process is vacuuming an index.

>Regards,

Sorry for the long delay on this. I have taken the approach as suggested
by Sawada-san and Robert and attached is v12.

1. The patch introduces a new counter in the same shared memory already
used by the parallel leader and workers to keep track of the number
of indexes completed. This way there is no reason to loop through
the index status every time we want to get the status of indexes completed.

2. A new function in vacuumparallel.c will be used to update
the progress of indexes completed by reading from the
counter created in point #1.

3. The function is called during the vacuum_delay_point as a
matter of convenience, since this is called in all major vacuum
loops. The function will only do something if the caller
sets a boolean to report progress. Doing so will also ensure
progress is being reported in case the parallel workers completed
before the leader.

4. Rather than adding any complexity to WaitForParallelWorkersToFinish
and introducing a new callback, vacuumparallel.c will wait until
the number of vacuum workers is 0 and then call
WaitForParallelWorkersToFinish as it does currently.

5. Went back to the idea of adding a new view called 
pg_stat_progress_vacuum_index
which is accomplished by adding a new type called VACUUM_PARALLEL in progress.h

Thanks,

Sami Imseih
Amazon Web Servies (AWS)

FYI: the above message was originally sent yesterday but 
was created under a separate thread. Please ignore this
thread[1]

[1]: 
https://www.postgresql.org/message-id/flat/4CD97E17-B9E4-421E-9A53-4317C90EFF35%40amazon.com










v12-0001--Show-progress-for-index-vacuums.patch
Description: v12-0001--Show-progress-for-index-vacuums.patch

Re: subtransaction performance

[Bruce Momjian]

On Mon, Oct 10, 2022 at 08:34:33PM -0700, Nathan Bossart wrote:
> On Mon, Oct 10, 2022 at 02:20:37PM -0400, Bruce Momjian wrote:
> > On Fri, Oct  7, 2022 at 03:23:27PM -0700, Zhihong Yu wrote:
> >> I wonder if SAVEPOINT / subtransaction performance has been boosted since 
> >> the
> >> blog was written.
> > 
> > No, I have not seen any changes in this area since then.  Seems there
> > are two problems --- the 64 cache per session and the 64k on the
> > replica.  In both cases, it seems sizing is not optimal, but sizing is
> > never optimal.  I guess we can look at allowing manual size adjustment,
> > automatic size adjustment, or a different approach that is more graceful
> > for larger savepoint workloads.
> 
> I believe the following commitfest entries might be relevant to this
> discussion:
> 
>   https://commitfest.postgresql.org/39/2627/
>   https://commitfest.postgresql.org/39/3514/
>   https://commitfest.postgresql.org/39/3806/

Wow, odd that I missed those.  Yes, they are very relevant.  :-)
The only other idea I had was to report such overflows, but these are
better.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: Query Jumbling for CALL and SET utility statements

[Imseih (AWS), Sami]

> I've been thinking since the beginning of this thread that there
> was no coherent, defensible rationale being offered for jumbling
> some utility statements and not others.

+1 to the idea, as there are other utility statement cases
that should be Jumbled. Here is a recent thread for jumbling
cursors [1].

The CF entry [2] has been withdrawn until consensus is reached
on this topic.

[1]: 
https://www.postgresql.org/message-id/203cfcf7-176e-4afc-a48e-b2cecfecd...@amazon.com
[2]: https://commitfest.postgresql.org/40/3901/


Regards,

Sami Imseih
AWS (Amazon Web Services)

Re: Frontend error logging style

[Dmitry Koval]

Hi!

Commit 9a374b77fb53 (Improve frontend error logging style.) replaced a 
line of file src/include/common/logging.h

```
extern PGDLLIMPORT enum pg_log_level __pg_log_level;
```
to
```
extern enum pg_log_level __pg_log_level;
```
i.e. it is rollback of commit 8ec569479fc28 (Apply PGDLLIMPORT markings 
broadly.)


Is it correct?

--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com

Make finding openssl program a configure or meson option

[Peter Eisentraut]

Various test suites use the "openssl" program as part of their setup. 
There isn't a way to override which openssl program is to be used, other 
than by fiddling with the path, perhaps.  This has gotten increasingly 
problematic with some of the work I have been doing, because different 
versions of openssl have different capabilities and do different things 
by default.  This patch checks for an openssl binary in configure and 
meson setup, with appropriate ways to override it.  This is similar to 
how "lz4" and "zstd" are handled, for example.  The meson build system 
actually already did this, but the result was only used in some places. 
This is now applied more uniformly.
From 70a115310a4f130751c0f3b4fcee69a9f29a2c3e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 11 Oct 2022 17:04:42 +0200
Subject: [PATCH] Make finding openssl program a configure or meson option

---
 configure | 55 +++
 configure.ac  |  1 +
 meson.build   |  1 +
 meson_options.txt |  3 +
 src/Makefile.global.in|  1 +
 src/test/ldap/Makefile|  1 +
 src/test/ldap/meson.build |  5 +-
 src/test/ldap/t/001_auth.pl   |  8 ++-
 .../modules/ssl_passphrase_callback/Makefile  |  4 +-
 .../ssl_passphrase_callback/meson.build   |  2 -
 src/test/ssl/Makefile |  2 +-
 src/test/ssl/meson.build  |  5 +-
 src/test/ssl/sslfiles.mk  | 34 ++--
 src/test/ssl/t/001_ssltests.pl|  2 +-
 14 files changed, 96 insertions(+), 28 deletions(-)

diff --git a/configure b/configure
index e04ee9fb4166..dd0802844a4a 100755
--- a/configure
+++ b/configure
@@ -648,6 +648,7 @@ PG_CRC32C_OBJS
 CFLAGS_ARMV8_CRC32C
 CFLAGS_SSE42
 LIBOBJS
+OPENSSL
 ZSTD
 LZ4
 UUID_LIBS
@@ -14023,6 +14024,60 @@ done
 
 fi
 
+if test -z "$OPENSSL"; then
+  for ac_prog in openssl
+do
+  # Extract the first word of "$ac_prog", so it can be a program name with 
args.
+set dummy $ac_prog; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
+$as_echo_n "checking for $ac_word... " >&6; }
+if ${ac_cv_path_OPENSSL+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  case $OPENSSL in
+  [\\/]* | ?:[\\/]*)
+  ac_cv_path_OPENSSL="$OPENSSL" # Let the user override the test with a path.
+  ;;
+  *)
+  as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
+for as_dir in $PATH
+do
+  IFS=$as_save_IFS
+  test -z "$as_dir" && as_dir=.
+for ac_exec_ext in '' $ac_executable_extensions; do
+  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
+ac_cv_path_OPENSSL="$as_dir/$ac_word$ac_exec_ext"
+$as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" 
>&5
+break 2
+  fi
+done
+  done
+IFS=$as_save_IFS
+
+  ;;
+esac
+fi
+OPENSSL=$ac_cv_path_OPENSSL
+if test -n "$OPENSSL"; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $OPENSSL" >&5
+$as_echo "$OPENSSL" >&6; }
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+fi
+
+
+  test -n "$OPENSSL" && break
+done
+
+else
+  # Report the value of OPENSSL in configure's output in all cases.
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for OPENSSL" >&5
+$as_echo_n "checking for OPENSSL... " >&6; }
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $OPENSSL" >&5
+$as_echo "$OPENSSL" >&6; }
+fi
+
 if test "$with_ssl" = openssl ; then
   ac_fn_c_check_header_mongrel "$LINENO" "openssl/ssl.h" 
"ac_cv_header_openssl_ssl_h" "$ac_includes_default"
 if test "x$ac_cv_header_openssl_ssl_h" = xyes; then :
diff --git a/configure.ac b/configure.ac
index f146c8301ae1..2b11d5016684 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1539,6 +1539,7 @@ if test "$with_gssapi" = yes ; then
[AC_CHECK_HEADERS(gssapi.h, [], [AC_MSG_ERROR([gssapi.h header file is 
required for GSSAPI])])])
 fi
 
+PGAC_PATH_PROGS(OPENSSL, openssl)
 if test "$with_ssl" = openssl ; then
   AC_CHECK_HEADER(openssl/ssl.h, [], [AC_MSG_ERROR([header file 
 is required for OpenSSL])])
   AC_CHECK_HEADER(openssl/err.h, [], [AC_MSG_ERROR([header file 
 is required for OpenSSL])])
diff --git a/meson.build b/meson.build
index 925db70c9d56..b124446277c0 100644
--- a/meson.build
+++ b/meson.build
@@ -324,6 +324,7 @@ tar = find_program(get_option('TAR'), native: true)
 gzip = find_program(get_option('GZIP'), native: true)
 program_lz4 = find_program(get_option('LZ4'), native: true, required: false)
 touch = find_program('touch', native: true)
+openssl = find_program(get_option('OPENSSL'), native: true, required: false)
 program_zstd = find_program(get_option('ZSTD'), native: true, required: false)
 dtrace = find_program(get_option('DTRACE'), native: true, required: 
get_option('dtrace'))
 missing = find_program('config/missing', native: true)
diff --git a/meson_options.txt b/meson_options.

Re: Frontend error logging style

[Tom Lane]

Dmitry Koval  writes:
> Hi!
> Commit 9a374b77fb53 (Improve frontend error logging style.) replaced a 
> line of file src/include/common/logging.h
> ```
> extern PGDLLIMPORT enum pg_log_level __pg_log_level;
> ```
> to
> ```
> extern enum pg_log_level __pg_log_level;
> ```
> i.e. it is rollback of commit 8ec569479fc28 (Apply PGDLLIMPORT markings 
> broadly.)

> Is it correct?

Yes, since that file is frontend-only.

regards, tom lane

Re: Warning about using pg_stat_reset() and pg_stat_reset_shared()

[Bruce Momjian]

On Wed, Oct  5, 2022 at 11:07:49AM +1300, David Rowley wrote:
> On Thu, 29 Sept 2022 at 04:45, Bruce Momjian  wrote:
> >
> > We have discussed the problems caused by the use of pg_stat_reset() and
> > pg_stat_reset_shared(), specifically the removal of information needed
> > by autovacuum.  I don't see these risks documented anywhere.  Should we
> > do that?  Are there other risks?
> 
> There was some discussion in [1] a few years back.  A few people were
> for the warning. Nobody seemed to object to it.  There's a patch in
> [2].
> 
> David
> 
> [1] 
> https://www.postgresql.org/message-id/flat/CAKJS1f8DTbCHf9gedU0He6ARsd58E6qOhEHM1caomqj_r9MOiQ%40mail.gmail.com
> [2] 
> https://www.postgresql.org/message-id/CAKJS1f80o98hcfSk8j%3DfdN09S7Sjz%2BvuzhEwbyQqvHJb_sZw0g%40mail.gmail.com

Ah, good point.  I have slightly reworded the doc patch, attached. 
However, the last line has me confused:

A database-wide ANALYZE is recommended after
the statistics have been reset.

As far as I can tell, analyze updates pg_statistics values, but not
pg_stat_all_tables.n_dead_tup and n_live_tup, which are used by
autovacuum to trigger vacuum operations.  I am afraid we have to
recommand VACUUM ANALYZE after pg_stat_reset(), no?

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 342b20ebeb..a404738fc4 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5480,6 +5480,17 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
 

 
+  
+   
+Using pg_stat_reset() also resets counters that
+autovacuum uses to determine when to trigger a vacuum or an analyze.
+Resetting these counters can cause autovacuum to not perform necessary
+work, which can cause problems such as table bloat or out-dated
+table statistics.  A database-wide ANALYZE is
+recommended after the statistics have been reset.
+   
+  
+
   
pg_stat_get_activity, the underlying function of
the pg_stat_activity view, returns a set of records

Re: Expand palloc/pg_malloc API

[Peter Eisentraut]

On 14.09.22 06:53, Tom Lane wrote:

I wrote:

It kind of feels that the argument order should be pointer, oldsize, size.
It feels even more strongly that people will get the ordering wrong,
whichever we choose.  Is there a way to make that more bulletproof?


Actually ... an even-more-terrifyingly-plausible misuse is that the
supplied oldsize is different from the actual previous allocation.
We should try to check that.  In MEMORY_CONTEXT_CHECKING builds
it should be possible to assert that oldsize == requested_size.
We don't have that data if !MEMORY_CONTEXT_CHECKING, but we could
at least assert that oldsize <= allocated chunk size.


I'm not very familiar with MEMORY_CONTEXT_CHECKING.  Where would one get 
these values?

Re: Make EXPLAIN generate a generic plan for a parameterized query

[Julien Rouhaud]

On Tue, Oct 11, 2022 at 09:49:14AM -0400, Tom Lane wrote:
>
> If you're trying to investigate custom-plan behavior, then you
> need to supply concrete parameter values somewhere, so I think
> this approach is fine for that case.  (Shoehorning parameter
> values into EXPLAIN options seems like it'd be a bit much.)
> However, investigating generic-plan behavior this way is tedious,
> since you have to invent irrelevant parameter values, plus mess
> with plan_cache_mode or else run the explain half a dozen times.
> So I can get behind having a more convenient way for that.

One common use case is tools identifying a slow query using pg_stat_statements,
identifying some missing indexes and then wanting to check whether the index
should be useful using some hypothetical index.

FTR I'm working on such a project and for now we have to go to great lengths
trying to "unjumble" such queries, so having a way to easily get the answer for
a generic plan would be great.

Re: Expand palloc/pg_malloc API

[Tom Lane]

Peter Eisentraut  writes:
> On 14.09.22 06:53, Tom Lane wrote:
>> Actually ... an even-more-terrifyingly-plausible misuse is that the
>> supplied oldsize is different from the actual previous allocation.
>> We should try to check that.  In MEMORY_CONTEXT_CHECKING builds
>> it should be possible to assert that oldsize == requested_size.
>> We don't have that data if !MEMORY_CONTEXT_CHECKING, but we could
>> at least assert that oldsize <= allocated chunk size.

> I'm not very familiar with MEMORY_CONTEXT_CHECKING.  Where would one get 
> these values?

Hmm ... the individual allocators have that info, but mcxt.c doesn't
have access to it.  I guess we could invent an additional "method"
to return the requested size of a chunk, which is only available in
MEMORY_CONTEXT_CHECKING builds, or maybe in !MEMORY_CONTEXT_CHECKING
it returns the allocated size instead.

regards, tom lane

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Alvaro Herrera]

On 2022-Oct-11, Richard Guo wrote:

> As I can see in 7fcbf6a4 ValidXLogPageHeader() is refactored as

True, but look at dfda6ebaec67 -- that changed the use of recaddr from
being built from parts (which were globals back then) into something
that was computed locally.

Knowing how difficult that code was, and how heroic was to change it to
a more maintainable form, I place no blame on failing to notice that
some small thing could have been written more easily.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Zhihong Yu]

On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval  wrote:

> Hi!
>
> Fixed couple warnings (for cfbot).
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com

Hi,
For v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch:

+   if (equal(name, cmd->name))
+   /* One new partition can have the same name as merged
partition. */
+   isSameName = true;

I think there should be a check before assigning true to isSameName - if
isSameName is true, that means there are two partitions with this same name.

Cheers

Re: Clarifying docs on nuance of select and update policies

[Bruce Momjian]

Thread moved to hackers since it involves complex queries.  Can someone
like Stephen comment on the existing docs and feature behavior?  Thanks.

---

On Fri, Sep 16, 2022 at 11:57:25AM -0700, Chandler Gonzales wrote:
> Hi y'all, I've got a proposed clarification to the documentation on the 
> nuances
> of RLS behavior for update policies, and maybe a (humble) request for a change
> in behavior to make it more intuitive. I am starting with pgsql-docs since I
> think the documentation change is a good starting point.
> 
> We use RLS policies to hide "soft deleted" objects from certain DB roles. We
> recently tried to add the ability to let a user "soft delete" a row. Ideally,
> we can write an RLS policy that allows a user to "soft delete" a row, but then
> hides the row from that same user once it is soft deleted.
> 
> Here's the setup on a fresh Postgres db for my example. I'm executing these
> queries as the database owner:
> ```
> create role some_user_type;
> create table foo(id int primary key, soft_deleted_at timestamptz,
> some_other_field text);
> alter table foo enable row level security;
> grant select on table foo to some_user_type;
> grant update(soft_deleted_at) on table foo to some_user_type;
> insert into foo(id) values (1);
> insert into foo(id, soft_deleted_at) values (2, now());
> ```
> 
> The behavior I'm trying to encode in RLS is that users with the role
> some_user_type can see all rows where soft_deleted_at is null, can update rows
> where BOTH the original soft_deleted_at is null AND the updated row has a
> non-null soft_deleted_at. Basically, the only thing this user can do to this
> row is to soft delete it.
> 
> We'll use a restrictive policy to get better error messages when we do an
> update later on:
> ```
> create policy pol_1 on foo for select to some_user_type using (true);
> create policy pol_1_res on foo as restrictive for select to some_user_type
> using (soft_deleted_at is null);
> ```
> 
> And just to verify it's working:
> ```
> chandler@localhost:chandler> begin; set local role some_user_type; select *
> from foo; rollback;
> BEGIN
> SET
> ╒╤═╤══╕
> │ id │ soft_deleted_at │ some_other_field │
> ╞╪═╪══╡
> │ 1  │ ¤               │ ¤                │
> ╘╧═╧══╛
> SELECT 1
> ROLLBACK
> ```
> 
> Now the important bit, the update policy:
> ```
> create policy pol_2 on foo for update to some_user_type using (soft_deleted_at
> is null) with check (soft_deleted_at is not null);
> ```
> 
> If we update a row without a where clause that touches the row, the update
> succeeds:
> ```
> chandler@localhost:chandler> begin; set local role some_user_type; update foo
> set soft_deleted_at = now() where true; rollback;
> BEGIN
> SET
> UPDATE 1
> ROLLBACK
> ```
> 
> But if we update a row with a where clause that uses the existing row, the
> update fails:
> ```
> chandler@localhost:chandler> begin; set local role some_user_type; update foo
> set soft_deleted_at = now() where id = 1; rollback;
> BEGIN
> SET
> new row violates row-level security policy "pol_1_res" for table "foo"
> ```
> 
> This was very unintuitive to me. My understanding is that when USING and WITH
> CHECK are both used for an update policy, the USING is tested against the
> original row, and the WITH CHECK clause against the new row.  This is stated 
> in
> the [documentation](https://www.postgresql.org/docs/current/
> sql-createpolicy.html):
> 
> > The USING expression determines which records the UPDATE command will see to
> operate against, while the WITH CHECK expression defines which modified rows
> are allowed to be stored back into the relation
> 
> So why is it that the addition of where id = 1 violates the select policy?
> Later in the same doc:
> 
> > Typically an UPDATE command also needs to read data from columns in the
> relation being updated (e.g., in a WHERE clause or a RETURNING clause, or in 
> an
> expression on the right hand side of the SET clause). In this case, SELECT
> rights are also required on the relation being updated, and the appropriate
> SELECT or ALL policies will be applied in addition to the UPDATE policies. 
> Thus
> the user must have access to the row(s) being updated through a SELECT or ALL
> policy in addition to being granted permission to update the row(s) via an
> UPDATE or ALL policy.
> 
> If you read this documentation perfectly literally, it does describe the
> behavior shown my examples above, but it is a bit ambiguous. I think a more
> clear explanation of this behavior would be the following:
> 
> > Typically an UPDATE command also needs to read data from columns in the
> relation being updated (e.g., in a WHERE clause or a RETURNING clause, or in 
> an
> expression on the right hand side of the SET clause). In this case, SELECT
> rights are also required on the relation being updated, and the

Re: Add a missing comment for PGPROC.pgprocno

[Bruce Momjian]

On Thu, Sep  8, 2022 at 08:16:07PM +0300, Aleksander Alekseev wrote:
> Hi hackers,
> 
> I tried to understand some implementation details of ProcArray and
> discovered that this is a bit challenging to do due to a missing
> comment for PGPROC.pgprocno. E.g. it's hard to understand why
> ProcArrayAdd() preserves procArray->pgprocnos[] sorted by (PGPROC *)
> if actually the sorting is done by pgprocno. Took me some time to
> figure this out.
> 
> Here is the patch that fixes this. Hopefully this will save some time
> for the newcomers.

Thanks, patch applied to master.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Zhihong Yu]

On Tue, Oct 11, 2022 at 9:58 AM Zhihong Yu  wrote:

>
>
> On Tue, Oct 11, 2022 at 9:22 AM Dmitry Koval 
> wrote:
>
>> Hi!
>>
>> Fixed couple warnings (for cfbot).
>>
>> --
>> With best regards,
>> Dmitry Koval
>>
>> Postgres Professional: http://postgrespro.com
>
> Hi,
> For v12-0001-PGPRO-ALTER-TABLE-MERGE-PARTITIONS-command.patch:
>
> +   if (equal(name, cmd->name))
> +   /* One new partition can have the same name as merged
> partition. */
> +   isSameName = true;
>
> I think there should be a check before assigning true to isSameName - if
> isSameName is true, that means there are two partitions with this same name.
>
> Cheers
>

Pardon - I see that transformPartitionCmdForMerge() compares the partition
names.
Maybe you can add a comment in ATExecMergePartitions referring to
transformPartitionCmdForMerge() so that people can more easily understand
the logic.

Re: Eliminating SPI from RI triggers - take 2

[Robert Haas]

On Thu, Sep 29, 2022 at 12:47 AM Amit Langote  wrote:
> [ patches ]

While looking over this thread I came across this code:

/* For data reading, executor always omits detached partitions */
if (estate->es_partition_directory == NULL)
estate->es_partition_directory =
CreatePartitionDirectory(estate->es_query_cxt, false);

But CreatePartitionDirectory is declared like this:

extern PartitionDirectory CreatePartitionDirectory(MemoryContext mcxt,
bool omit_detached);

So the comment seems to say the opposite of what the code does. The
code seems to match the explanation in the commit message for
71f4c8c6f74ba021e55d35b1128d22fb8c6e1629, so I am guessing that
perhaps s/always/never/ is needed here.

I also noticed that ExecCreatePartitionPruneState no longer exists in
the code but is still referenced in
src/test/modules/delay_execution/specs/partition-addition.spec

Regarding 0003, it seems unfortunate that
find_inheritance_children_extended() will now have 6 arguments 4 of
which have to do with detached partition handling. That is a lot of
detached partition handling, and it's hard to reason about. I don't
see an obvious way of simplifying things very much, but I wonder if we
could at least have the new omit_detached_snapshot snapshot replace
the existing bool omit_detached flag. Something like the attached
incremental patch.

Probably we need to go further than the attached, though. I don't
think that PartitionDirectoryLookup() should be getting any new
arguments. The whole point of that function is that it's supposed to
ensure that the returned value is stable, and the comments say so. But
with these changes it isn't any more, because it depends on the
snapshot you pass. It seems fine to specify when you create the
partition directory that you want it to show a different, still-stable
view of the world, but as written, it seems to me to undermine the
idea that the return value is expected to be stable at all. Is there a
way we can avoid that?

-- 
Robert Haas
EDB: http://www.enterprisedb.com
diff --git a/src/backend/catalog/pg_inherits.c 
b/src/backend/catalog/pg_inherits.c
index f810e5de0d..eb5377e7c0 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -60,7 +60,7 @@ typedef struct SeenRelsEntry
 List *
 find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
 {
-   return find_inheritance_children_extended(parentrelId, true,
+   return find_inheritance_children_extended(parentrelId,

  ActiveSnapshotSet() ?

  GetActiveSnapshot() : NULL,

  lockmode, NULL, NULL);
@@ -75,7 +75,7 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
  * If a partition's pg_inherits row is marked "detach pending",
  * *detached_exist (if not null) is set true.
  *
- * If omit_detached is true and the caller passed 'omit_detached_snapshot',
+ * If the caller passed 'omit_detached_snapshot',
  * the partition whose pg_inherits tuple marks it as "detach pending" is
  * omitted from the output list if the tuple is visible to that snapshot.
  * That is, such a partition is omitted from the output list depending on
@@ -84,7 +84,7 @@ find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
  * NULL) is set to the xmin of that pg_inherits tuple.
  */
 List *
-find_inheritance_children_extended(Oid parentrelId, bool omit_detached,
+find_inheritance_children_extended(Oid parentrelId,
   Snapshot 
omit_detached_snapshot,
   LOCKMODE 
lockmode, bool *detached_exist,
   
TransactionId *detached_xmin)
@@ -146,7 +146,7 @@ find_inheritance_children_extended(Oid parentrelId, bool 
omit_detached,
if (detached_exist)
*detached_exist = true;
 
-   if (omit_detached && omit_detached_snapshot)
+   if (omit_detached_snapshot)
{
TransactionId xmin;
 
diff --git a/src/backend/partitioning/partdesc.c 
b/src/backend/partitioning/partdesc.c
index 863b04c17d..23f1334dbc 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -48,7 +48,6 @@ typedef struct PartitionDirectoryEntry
 } PartitionDirectoryEntry;
 
 static PartitionDesc RelationBuildPartitionDesc(Relation rel,
-   
bool omit_detached,

Snapshot omit_detached_snapshot);
 
 
@@ -76,8

Re: use has_privs_of_role() for pg_hba.conf

[Nathan Bossart]

On Tue, Oct 11, 2022 at 02:01:07PM +0900, Michael Paquier wrote:
> Thanks, applied.  It took me a few minutes to note that
> regress_regression_* is required in the object names because we need
> to use the same name for the parent role and the database, with
> "regress_" being required for the role and "regression" being required
> for the database.  I have added an extra section where pg_hba.conf is
> set to match only the parent role, while on it.  perltidy has reshaped
> things in an interesting way, because the generated log_[un]like is
> long, it seems.

Thanks!

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

Re: Suppressing useless wakeups in walreceiver

[Nathan Bossart]

On Tue, Oct 11, 2022 at 09:34:25AM +0530, Bharath Rupireddy wrote:
> now = t1;
> XLogWalRcvSendReply() /* say it ran for a minute or so for whatever reasons */
> XLogWalRcvSendHSFeedback() /* with patch walrecevier sends hot standby
> feedback more often without properly honouring
> wal_receiver_status_interval because the 'now' isn't actually the
> current time as far as that function is concerned, it is
> t1 + XLogWalRcvSendReply()'s time. */
> 
> Well, is this really a problem? I'm not sure about that. Let's hear from 
> others.

For this example, the feedback message would just be sent in the next loop
iteration instead.

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

Re: Mingw task for Cirrus CI

[Andres Freund]

Hi,

On 2022-10-07 20:00:56 +0300, Melih Mutlu wrote:
> Attached patch is the adjusted version to build with meson.

Thanks!


> diff --git a/.cirrus.yml b/.cirrus.yml
> index 9f2282471a..7e6ebdf7b7 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -35,6 +35,7 @@ on_failure_ac: &on_failure_ac
>- "**/*.log"
>- "**/*.diffs"
>- "**/regress_log_*"
> +  - "**/*.stackdump"
>  type: text/plain


I think it might be easier to just set MSYS=winjitdebug
https://www.msys2.org/wiki/JIT-Debugging/#native-windows-processes-started-from-msys2
and then rely on the existing windows crash reporting stuff.

>setup_additional_packages_script: |
>  REM choco install -y --no-progress ...
> -
># Use /DEBUG:FASTLINK to avoid high memory usage during linking
>configure_script: |

You removed a bunch of newlines here and nearby - I assume that wasn't
intentional?


> +  mingw_info_script:
> +- C:\msys64\usr\bin\dash.exe -lc "where gcc"
> +- C:\msys64\usr\bin\dash.exe -lc "gcc --version"
> +- C:\msys64\usr\bin\dash.exe -lc "where perl"
> +- C:\msys64\usr\bin\dash.exe -lc "perl --version"
> +
> +  configure_script:
> +- C:\msys64\usr\bin\dash.exe -lc "cd %CIRRUS_WORKING_DIR% &&
> +  meson setup --buildtype debug -Dcassert=true -Db_pch=true -DTAR=%TAR% 
> build"

There's no need to use dash anymore, the amount of shell script run is
minimal.


> +  build_script:
> +- C:\msys64\usr\bin\dash.exe -lc "cd %CIRRUS_WORKING_DIR% && ninja -C 
> %BUILD_DIR%"
> +  upload_caches: ccache
> +
> +  test_world_script:
> +- C:\msys64\usr\bin\dash.exe -lc "cd %CIRRUS_WORKING_DIR% && meson test 
> --print-errorlogs --num-processes %TEST_JOBS% -C %BUILD_DIR%"
> +

I think the "cd"s here are superfluous given the ninja -C %BUILD_DIR% etc.

Greetings,

Andres Freund

Re: doc: add entry for pg_get_partkeydef()

[Tom Lane]

Ian Lawrence Barwick  writes:
> Small patch for $subject, as the other pg_get_XXXdef() functions are
> documented
> and I was looking for this one but couldn't remember what it was called.

Seems reasonable.  Pushed with minor wording adjustment.

regards, tom lane

Re: Mingw task for Cirrus CI

[Andres Freund]

Hi,

On 2022-10-11 11:23:36 -0700, Andres Freund wrote:
> > +  build_script:
> > +- C:\msys64\usr\bin\dash.exe -lc "cd %CIRRUS_WORKING_DIR% && ninja -C 
> > %BUILD_DIR%"
> > +  upload_caches: ccache

Only remembered that just after sending my email: When using b_pch=true (which
saves a lot of time on mingw) ccache won't cache much (IIRC just the pch files
themselves) unless you do something like
export CCACHE_SLOPPINESS=pch_defines,time_macros

$ ccache -z -C
Clearing... 100.0% 
[==]

$ ninja clean && ccache -z && CCACHE_SLOPPINESS= time ninja 
src/bin/pg_test_fsync/pg_test_fsync.exe && ccache -s
[2/2] Cleaning
Cleaning... 200 files.
Statistics zeroed
[124/124] Linking target src/bin/pg_test_fsync/pg_test_fsync.exe
0.00user 0.01system 0:07.60elapsed 0%CPU (0avgtext+0avgdata 4936maxresident)k
0inputs+0outputs (1314major+0minor)pagefaults 0swaps
Summary:
  Hits:   0 /1 (0.00 %)
Direct:   0 /1 (0.00 %)
Preprocessed: 0 /1 (0.00 %)
  Misses: 1
Direct:   1
Preprocessed: 1
  Uncacheable:  110

As you can see, most of the files are determined to be unachable. The build
took 7.6s.


$ ninja clean && ccache -z && CCACHE_SLOPPINESS=pch_defines,time_macros time 
ninja src/bin/pg_test_fsync/pg_test_fsync.exe && ccache -s
[2/2] Cleaning
Cleaning... 200 files.
Statistics zeroed
[124/124] Linking target src/bin/pg_test_fsync/pg_test_fsync.exe
0.00user 0.01system 0:09.91elapsed 0%CPU (0avgtext+0avgdata 4936maxresident)k
0inputs+0outputs (1313major+0minor)pagefaults 0swaps
Summary:
  Hits:   0 /  111 (0.00 %)
Direct:   0 /  111 (0.00 %)
Preprocessed: 0 /  111 (0.00 %)
  Misses:   111
Direct: 111
Preprocessed:   111
Primary storage:
  Hits:   0 /  216 (0.00 %)
  Misses:   216
  Cache size (GB): 0.05 / 5.00 (0.98 %)

Files are cachable, but are cache misses (because we cleaned the cache
above). The build took 9.91s.


$ ninja clean && ccache -z && CCACHE_SLOPPINESS=pch_defines,time_macros time 
ninja src/bin/pg_test_fsync/pg_test_fsync.exe && ccache -s
[2/2] Cleaning
Cleaning... 200 files.
Statistics zeroed
[124/124] Linking target src/bin/pg_test_fsync/pg_test_fsync.exe
0.00user 0.01system 0:02.40elapsed 0%CPU (0avgtext+0avgdata 4936maxresident)k
0inputs+0outputs (1314major+0minor)pagefaults 0swaps
Summary:
  Hits: 111 /  111 (100.0 %)
Direct: 111 /  111 (100.0 %)
Preprocessed: 0 /0
  Misses: 0
Direct:   0
Preprocessed: 0
Primary storage:
  Hits: 222 /  222 (100.0 %)
  Misses: 0
  Cache size (GB): 0.05 / 5.00 (0.98 %)

Files are cachable, and hit the cache. The build takes 2.4s.


Using ccache's depend mode reduce the cache-miss case to 7.51s and the cache
hit case to 1.75s. So we should likely export CCACHE_DEPEND=1 as well.

Greetings,

Andres Freund

Re: Warning about using pg_stat_reset() and pg_stat_reset_shared()

[David Rowley]

On Wed, 12 Oct 2022 at 04:11, Bruce Momjian  wrote:
> As far as I can tell, analyze updates pg_statistics values, but not
> pg_stat_all_tables.n_dead_tup and n_live_tup, which are used by
> autovacuum to trigger vacuum operations.  I am afraid we have to
> recommand VACUUM ANALYZE after pg_stat_reset(), no?

As far as I can see ANALYZE will update these fields.  I'm looking at
pgstat_report_analyze() called from do_analyze_rel().

It does:

tabentry->n_live_tuples = livetuples;
tabentry->n_dead_tuples = deadtuples;

I also see it working from testing:

create table t as select x from generate_Series(1,10)x;
delete from t where x > 9;
select pg_sleep(1);
select n_live_tup,n_dead_tup from pg_stat_user_tables where relname = 't';
select pg_stat_reset();
select n_live_tup,n_dead_tup from pg_stat_user_tables where relname = 't';
analyze t;
select n_live_tup,n_dead_tup from pg_stat_user_tables where relname = 't';

The result of the final query is:

 n_live_tup | n_dead_tup
+
  9 |  1

Maybe the random sample taken by ANALYZE for your case didn't happen
to land on any pages with dead tuples?

David

Re: Query Jumbling for CALL and SET utility statements

[Michael Paquier]

On Tue, Oct 11, 2022 at 02:18:54PM +, Imseih (AWS), Sami wrote:
> +1 to the idea, as there are other utility statement cases
> that should be Jumbled. Here is a recent thread for jumbling
> cursors [1].

Thanks for mentioning that.  With an automated way to generate this
code, cursors would be handled, at the advantage of making sure that
no fields are missing in the jumbled structures (is_local was missed
for example on SET).

> The CF entry [2] has been withdrawn until consensus is reached
> on this topic.

It seems to me that the consensus is here, which is a good step
forward ;)
--
Michael


signature.asc
Description: PGP signature

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Michael Paquier]

On Tue, Oct 11, 2022 at 06:24:26PM +0200, Alvaro Herrera wrote:
> Knowing how difficult that code was, and how heroic was to change it to
> a more maintainable form, I place no blame on failing to notice that
> some small thing could have been written more easily.

+1.  And even after such changes the code is still complex for the
common eye.  Anyway, this makes the code a bit simpler with the exact
same maths, so applied.
--
Michael


signature.asc
Description: PGP signature

Re: Make finding openssl program a configure or meson option

[Michael Paquier]

On Tue, Oct 11, 2022 at 05:06:22PM +0200, Peter Eisentraut wrote:
> Various test suites use the "openssl" program as part of their setup. There
> isn't a way to override which openssl program is to be used, other than by
> fiddling with the path, perhaps.  This has gotten increasingly problematic
> with some of the work I have been doing, because different versions of
> openssl have different capabilities and do different things by default.
> This patch checks for an openssl binary in configure and meson setup, with
> appropriate ways to override it.  This is similar to how "lz4" and "zstd"
> are handled, for example.  The meson build system actually already did this,
> but the result was only used in some places. This is now applied more
> uniformly.

openssl-env allows the use of the environment variable of the same
name.  This reminds me a bit of the recent interferences with GZIP,
for example.

This patch is missing one addition of set_single_env() in
vcregress.pl, and one update of install-windows.sgml where all the
supported environment variables for commands are listed.
--
Michael


signature.asc
Description: PGP signature

Re: Allow tests to pass in OpenSSL FIPS mode

[Michael Paquier]

On Tue, Oct 11, 2022 at 01:51:50PM +0200, Peter Eisentraut wrote:
> Let's make a small start on this.  The attached patch moves the tests of the
> md5() function to a separate test file.  That would ultimately make it
> easier to maintain a variant expected file for FIPS mode where that function
> will fail (similar to how we have done it for the pgcrypto tests).

Makes sense to me.  This slice looks fine.

I think that the other md5() computations done in the main regression
test suite could just be switched to use one of the sha*() functions
as they just want to put their hands on text values.  It looks like a
few of them have some expections with the output size and
generate_series(), though, but this could be tweaked by making the
series shorter, for example.
--
Michael


signature.asc
Description: PGP signature

RE: Allow logical replication to copy tables in binary format

[osumi.takami...@fujitsu.com]

Hi,


On Monday, October 3, 2022 8:50 PM Melih Mutlu  wrote:
>   (4) Error message of the binary format copy
> 
>   I've gotten below message from data type contradiction (between
> integer and bigint).
>   Probably, this is unclear for the users to understand the direct cause
>   and needs to be adjusted ?
>   This might be a similar comment Euler mentioned in [1].
> 
>   2022-09-16 11:54:54.835 UTC [4570] ERROR:  insufficient data left in
> message
>   2022-09-16 11:54:54.835 UTC [4570] CONTEXT:  COPY tab, line 1,
> column id
>
> It's already unclear for users to understand what's the issue if they're 
> copying
> data between different column types via the COPY command.
> This issue comes from COPY, and logical replication just utilizes COPY.
> I don't think it would make sense to adjust an error message from a 
> functionality
> which logical replication only uses and has no direct impact on.
> It might be better to do this in a separate patch. What do you think?
Yes, makes sense. It should be a separate patch.


>   I agree with the direction to support binary copy for v16 and later.
> 
>   IIUC, the binary format replication with different data types fails even
> during apply phase on HEAD.
>   I thought that means, the upgrade concern only applies to a scenario
> that the user executes
>   only initial table synchronizations between the publisher and subscriber
>   and doesn't replicate any data at apply phase after that. I would say
>   this isn't a valid scenario and your proposal makes sense.
> 
> No, logical replication in binary does not fail on apply phase if data types 
> are
> different.
With HEAD, I observe in some case we fail at apply phase because of different 
data types like 
integer vs. bigint as written scenario in [1]. In short, I think we can slightly
adjust your documentation and make it more general so that the description 
applies to
both table sync phase and apply phase.

I'll suggest a below change for your sentence of logical-replication.sgml.
FROM:
In binary case, it is not allowed to replicate data between different types due 
to restrictions inherited from COPY.
TO:
Binary format is type specific and does not allow to replicate data between 
different types according to its
restrictions.


If my idea above is correct, then I feel we can remove all the fixes for 
create_subscription.sgml.
I'm not sure if I should pursue this perspective of the document improvement
any further after this email, since this isn't essentially because of this 
patch.



> The concern with upgrade (if data types are not the same) would be not being
> able to create a new subscription with binary enabled or replicate new tables
> added into publication.
> Replication of tables from existing subscriptions would not be affected by 
> this
> change since they will already be in the apply phase, not tablesync.
> Do you think this would still be an issue?
Okay, thanks for explaining this. I understand that
the upgrade concern applies to the table sync that is executed
between text format (before the patch) and binary format (after the patch).




[1] - binary format test that we fail for different types on apply phase on HEAD


create table tab (id integer);
insert into tab values (100);
create publication mypub for table tab;


create table tab (id bigint);
create subscription mysub connection '...' publication mypub with (copy_data = 
false, binary = true, disable_on_error = true);

-- wait for several seconds


select srsubid, srrelid, srrelid::regclass, srsubstate, srsublsn from 
pg_subscription_rel; -- check the status as 'r' for the relation
select * from tab; -- confirm we don't copy the initial data on the pub


insert into tab values (1), (2);

-- wait for several seconds


select subname, subenabled from pg_subscription; -- shows 'f' for the 2nd 
column because of an error
select * from tab -- no records

This error doesn't happen when we adopt 'integer' on the subscriber aligned 
with the publisher
and we can see the two records on the subscriber.



Best Regards,
Takamichi Osumi

Re: shadow variables - pg15 edition

[Michael Paquier]

On Tue, Oct 11, 2022 at 01:16:50PM +1300, David Rowley wrote:
> Aside from this issue, if anything I'd be keen to go a little further
> with this and upgrade to -Wshadow=local. The reason being is that I
> noticed that the const qualifier is not classed as "compatible" with
> the equivalently named and typed variable without the const qualifier.
> ISTM that there's close to as much opportunity to mix up two variables
> with the same name that are const and non-const as there are two
> variables with the same const qualifier. However, I'll be waiting for
> the dust to settle on the current flags before thinking any more about
> that.

-Wshadow=compatible-local causes one extra warning in postgres.c with
-DWRITE_READ_PARSE_PLAN_TREES:
postgres.c: In function ‘pg_rewrite_query’:
postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter 
[-Wshadow=compatible-local]
  818 | Query  *query = lfirst_node(Query, lc);
  | ^
postgres.c:771:25: note: shadowed declaration is here
  771 | pg_rewrite_query(Query *query)
  |  ~~~^

Something like the patch attached would deal with this one.
--
Michael
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 5352d5f4c6..27dee29f42 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -815,15 +815,15 @@ pg_rewrite_query(Query *query)
 
 		foreach(lc, querytree_list)
 		{
-			Query	   *query = lfirst_node(Query, lc);
-			char	   *str = nodeToString(query);
+			Query	   *curr_query = lfirst_node(Query, lc);
+			char	   *str = nodeToString(curr_query);
 			Query	   *new_query = stringToNodeWithLocations(str);
 
 			/*
 			 * queryId is not saved in stored rules, but we must preserve it
 			 * here to avoid breaking pg_stat_statements.
 			 */
-			new_query->queryId = query->queryId;
+			new_query->queryId = curr_query->queryId;
 
 			new_list = lappend(new_list, new_query);
 			pfree(str);


signature.asc
Description: PGP signature

Re: shadow variables - pg15 edition

[David Rowley]

On Wed, 12 Oct 2022 at 14:39, Michael Paquier  wrote:
> -Wshadow=compatible-local causes one extra warning in postgres.c with
> -DWRITE_READ_PARSE_PLAN_TREES:
> postgres.c: In function ‘pg_rewrite_query’:
> postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter 
> [-Wshadow=compatible-local]
>   818 | Query  *query = lfirst_node(Query, lc);
>   | ^
> postgres.c:771:25: note: shadowed declaration is here
>   771 | pg_rewrite_query(Query *query)
>   |  ~~~^
>
> Something like the patch attached would deal with this one.

Thanks for finding that and coming up with the patch. It looks fine to
me. Do you want to push it?

David

Re: Issue in GIN fast-insert: XLogBeginInsert + Read/LockBuffer ordering

[Michael Paquier]

On Fri, Sep 30, 2022 at 06:22:02PM +0800, Zhang Mingli wrote:
> In the same function, there is disorder of XLogBeginInsert and 
> START_CRIT_SECTION.
> 
> ```
> collectordata = ptr = (char *) palloc(collector->sumsize);
> 
>  data.ntuples = collector->ntuples;
> 
>  if (needWal)
>    XLogBeginInsert();
> 
>  START_CRIT_SECTION();
> ```
> 
> Shall we adjust that too?

Nice catches, both of you.  Let's adjust everything spotted in one
shot.  Matthias' patch makes the ordering right, but the
initialization path a bit harder to follow when using a separate list.
The code is short so it does not strike me as a big deal, and it comes
from the fact that we need to lock an existing buffer when merging two
lists.  For the branch where we insert into a tail page, the pages are
already locked but it looks to be enough to move XLogBeginInsert()
before the first XLogRegisterBuffer() call.

Would any of you like to write a patch?
--
Michael


signature.asc
Description: PGP signature

Re: shadow variables - pg15 edition

[Michael Paquier]

On Wed, Oct 12, 2022 at 02:50:58PM +1300, David Rowley wrote:
> Thanks for finding that and coming up with the patch. It looks fine to
> me. Do you want to push it?

Thanks for double-checking.  I'll do so shortly, I just got annoyed by
that for a few days :)

Thanks for your work on this thread to be able to push the switch by
default, by the way.
--
Michael


signature.asc
Description: PGP signature

Re: Remove an unnecessary LSN calculation while validating WAL page header

[Richard Guo]

On Wed, Oct 12, 2022 at 12:24 AM Alvaro Herrera 
wrote:

> Knowing how difficult that code was, and how heroic was to change it to
> a more maintainable form, I place no blame on failing to notice that
> some small thing could have been written more easily.


Concur with that. The changes in 7fcbf6a4 made the code to a more
maintainable and readable state. It's a difficult but awesome work.

Thanks
Richard

Allow WindowFuncs prosupport function to use more optimal WindowClause options

[David Rowley]

Over on [1], Erwin mentions that row_number() over (ORDER BY ... ROWS
UNBOUNDED PRECEDING) is substantially faster than the default RANGE
UNBOUNDED PRECEDING WindowClause options.  The difference between
these options are that nodeWindowAgg.c checks for peer rows for RANGE
but not for ROWS.  That would make a difference for many of our
built-in window and aggregate functions, but row_number() does not
really care.

To quantify the performance difference, take the following example:

create table ab (a int, b int) ;
insert into ab
select x,y from generate_series(1,1000)x,generate_series(1,1000)y;
create index on ab(a,b);

--  range unbounded (the default)
explain (analyze, costs off, timing off)
select a,b from (select a,b,row_number() over (partition by a order by
a range unbounded preceding) rn from ab) ab where rn <= 1;
  QUERY PLAN
---
 Subquery Scan on ab (actual rows=1000 loops=1)
   ->  WindowAgg (actual rows=1000 loops=1)
 Run Condition: (row_number() OVER (?) <= 1)
 ->  Index Only Scan using ab_a_b_idx on ab ab_1 (actual
rows=100 loops=1)
   Heap Fetches: 100
 Planning Time: 0.091 ms
 Execution Time: 336.172 ms
(7 rows)

If that were switched to use ROWS UNBOUNDED PRECEDING then the
executor would not have to check for peer rows in the window frame.

explain (analyze, costs off, timing off)
select a,b from (select a,b,row_number() over (partition by a order by
a rows unbounded preceding) rn from ab) ab where rn <= 1;
  QUERY PLAN
---
 Subquery Scan on ab (actual rows=1000 loops=1)
   ->  WindowAgg (actual rows=1000 loops=1)
 Run Condition: (row_number() OVER (?) <= 1)
 ->  Index Only Scan using ab_a_b_idx on ab ab_1 (actual
rows=100 loops=1)
   Heap Fetches: 0
 Planning Time: 0.178 ms
 Execution Time: 75.101 ms
(7 rows)

Time: 75.673 ms (447% faster)

You can see that this executes quite a bit more quickly. As Erwin
pointed out to me (off-list), this along with the monotonic window
function optimisation that was added in PG15 the performance of this
gets close to DISTINCT ON.

explain (analyze, costs off, timing off)
select distinct on (a) a,b from ab order by a,b;
 QUERY PLAN

 Unique (actual rows=1000 loops=1)
   ->  Index Only Scan using ab_a_b_idx on ab (actual rows=100 loops=1)
 Heap Fetches: 0
 Planning Time: 0.071 ms
 Execution Time: 77.988 ms
(5 rows)

I've not really done any analysis into which other window functions
can use this optimisation. The attached only adds support to
row_number()'s support function and only converts exactly "RANGE
UNBOUNDED PRECEDING AND CURRENT ROW" into "ROW UNBOUNDED PRECEDING AND
CURRENT ROW".  That might need to be relaxed a little, but I've done
no analysis to find that out.   Erwin mentioned to me that he's not
currently in a position to produce a patch for this, so here's the
patch. I'm hoping the existence of this might coax Erwin into doing
some analysis into what other window functions we can support and what
other frame options can be optimised.

[1] 
https://postgr.es/m/caghenj7lbbszxs+skwwfvnbmot2ovsbhdmb1dfrgerceya_...@mail.gmail.com
diff --git a/src/backend/optimizer/plan/planner.c 
b/src/backend/optimizer/plan/planner.c
index 5d0fd6e072..e55e0e49a5 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -38,6 +38,7 @@
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
+#include "nodes/supportnodes.h"
 #ifdef OPTIMIZER_DEBUG
 #include "nodes/print.h"
 #endif
@@ -207,6 +208,8 @@ static PathTarget *make_partial_grouping_target(PlannerInfo 
*root,

PathTarget *grouping_target,

Node *havingQual);
 static List *postprocess_setop_tlist(List *new_tlist, List *orig_tlist);
+static void optimize_window_clause_frameoptions(PlannerInfo *root,
+   
WindowFuncLists *wflists);
 static List *select_active_windows(PlannerInfo *root, WindowFuncLists 
*wflists);
 static PathTarget *make_window_input_target(PlannerInfo *root,

PathTarget *final_target,
@@ -1422,7 +1425,15 @@ grouping_planner(PlannerInfo *root, double 
tuple_fraction)
wflists = find_window_functions((Node *) 
root->processed_tlist,

   

Re: Fast COPY FROM based on batch insert

[Etsuro Fujita]

On Tue, Oct 11, 2022 at 3:06 PM Andrey Lepikhov
 wrote:
> I reviewed the patch one more time. Only one question: bistate and
> ri_FdwRoutine are strongly bounded. Maybe to add some assertion on
> (ri_FdwRoutine XOR bistate) ? Just to prevent possible errors in future.

You mean the bistate member of CopyMultiInsertBuffer?

We do not use that member at all for foreign tables, so the patch
avoids initializing that member in CopyMultiInsertBufferInit() when
called for a foreign table.  So we have bistate = NULL for foreign
tables (and bistate != NULL for plain tables), as you mentioned above.
I think it is a good idea to add such assertions.  How about adding
them to CopyMultiInsertBufferFlush() and
CopyMultiInsertBufferCleanup() like the attached?  In the attached I
updated comments a bit further as well.

Thanks for reviewing!

Best regards,
Etsuro Fujita


v4-0001-Implementation-of-a-Bulk-COPY-FROM-efujita-5.patch
Description: Binary data

Re: test_decoding assertion failure for the loss of top-sub transaction relationship

[Masahiko Sawada]

On Wed, Sep 7, 2022 at 11:06 AM kuroda.hay...@fujitsu.com
 wrote:
>
> Dear Amit,
>
> Thanks for giving comments!
>
> > Did you get this new assertion failure after you applied the patch for
> > the first failure? Because otherwise, how can you reach it with the
> > same test case?
>
> The first failure is occurred only in the HEAD, so I did not applied the 
> first patch
> to REL14 and REL15.
> This difference is caused because the commit [Fix catalog lookup...] in 
> REL15(272248a) and older is different
> from the HEAD one.
> In order versions SnapBuildXidSetCatalogChanges() has been added. In the 
> function
> a transaction will be marked as containing catalog changes if the transaction 
> is in InitialRunningXacts,
> and after that the relation between sub-top transactions is assigned based on 
> the parsed->subxact.
> The marking avoids the first failure, but the assignment triggers new failure.
>
>
> > About patch:
> > else if (sub_needs_timetravel)
> >   {
> > - /* track toplevel txn as well, subxact alone isn't meaningful */
> > + elog(DEBUG2, "forced transaction %u to do timetravel due to one of
> > its subtransaction",
> > + xid);
> > + needs_timetravel = true;
> >   SnapBuildAddCommittedTxn(builder, xid);
> >
> > Why did you remove the above comment? I think it still makes sense to 
> > retain it.
>
> Fixed.

Here are some review comments for v2 patch:

+# Test that we can force the top transaction to do timetravel when one of sub
+# transactions needs that. This is necessary when we restart decoding
from RUNNING_XACT
+# without the wal to associate subtransaction to its top transaction.

I don't think the second sentence is necessary.

---
The last decoding
+# starts from the first checkpoint and NEW_CID of "s0_truncate"
doesn't mark the top
+# transaction as catalog modifying transaction. In this scenario, the
enforcement sets
+# needs_timetravel to true even if the top transaction is regarded as
that it does not
+# have catalog changes and thus the decoding works without a
contradition that one
+# subtransaction needed timetravel while its top transaction didn't.

I don't understand the last sentence, probably it's a long sentence.

How about the following description?

# Test that we can handle the case where only subtransaction is marked
as containing
# catalog changes. The last decoding starts from NEW_CID generated by
"s0_truncate" and
# marks only the subtransaction as containing catalog changes but we
don't create the
# association between top-level transaction and subtransaction yet.
When decoding the
# commit record of the top-level transaction, we must force the
top-level transaction
# to do timetravel since one of its subtransactions is marked as
containing catalog changes.

---
+ elog(DEBUG2, "forced transaction %u to do timetravel due to one of
its subtransaction",
+ xid);
+ needs_timetravel = true;

I think "one of its subtransaction" should be "one of its subtransactions".

Regards,

--
Masahiko Sawada

Re: Allow WindowFuncs prosupport function to use more optimal WindowClause options

[Vik Fearing]

On 10/12/22 04:40, David Rowley wrote:

I've not really done any analysis into which other window functions
can use this optimisation. The attached only adds support to
row_number()'s support function and only converts exactly "RANGE
UNBOUNDED PRECEDING AND CURRENT ROW" into "ROW UNBOUNDED PRECEDING AND
CURRENT ROW".  That might need to be relaxed a little, but I've done
no analysis to find that out. 


Per spec, the ROW_NUMBER() window function is not even allowed to have a 
frame specified.


b) The window framing clause of WDX shall not be present.

Also, the specification for ROW_NUMBER() is:

f) ROW_NUMBER() OVER WNS is equivalent to the :

COUNT (*) OVER (WNS1 ROWS UNBOUNDED PRECEDING)


So I don't think we need to test for anything at all and can 
indiscriminately add or replace the frame with ROWS UNBOUNDED PRECEDING.

--
Vik Fearing

RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

[kuroda.hay...@fujitsu.com]

Dear Önder,

Thank you for updating the patch!

> It is not about CREATE INDEX being async. It is about pg_stat_all_indexes
> being async. If we do not wait, the tests become flaky, because sometimes
> the update has not been reflected in the view immediately.

Make sense, I forgot how stats collector works...

Followings are comments for v16. Only for test codes.

~~~
01. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES 
AFTER ANALYZE

```
# show that index_b is not used
$node_subscriber->poll_query_until(
'postgres', q{select idx_scan=0 from pg_stat_all_indexes where 
indexrelname = 'index_b';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates 
two rows via index scan with index on high cardinality column-2";
```

poll_query_until() is still remained here, it should be replaced to is().


02. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
# show that the unique index on replica identity is used even when 
enable_indexscan=false
$result = $node_subscriber->safe_psql('postgres',
"select idx_scan from pg_stat_all_indexes where indexrelname = 
'test_replica_id_full_idx'");
is($result, qq(0), 'ensure subscriber has not used index with 
enable_indexscan=false');
```

Is the comment wrong? The index test_replica_id_full_idx is not used here.


032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER 
ANALYZE

```
# show that index_b is not used
$node_subscriber->poll_query_until(
'postgres', q{select idx_scan=0 from pg_stat_all_indexes where 
indexrelname = 'index_b';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates 
two rows via index scan with index on high cardinality column-2";
```

poll_query_until() is still remained here, it should be replaced to is()

032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
# show that the unique index on replica identity is used even when 
enable_indexscan=false
$result = $node_subscriber->safe_psql('postgres',
"select idx_scan from pg_stat_all_indexes where indexrelname = 
'test_replica_id_full_idx'");
is($result, qq(0), 'ensure subscriber has not used index with 
enable_indexscan=false');
```

Is the comment wrong? The index test_replica_id_full_idx is not used here.

03. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
$node_publisher->safe_psql('postgres',
"ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX 
test_replica_id_full_unique;");
```

I was not sure why ALTER TABLE REPLICA IDENTITY USING INDEX was done on the 
publisher side.
IIUC this feature works when REPLICA IDENTITY FULL is specified on a publisher,
so it might not be altered here. If so, an index does not have to define on the 
publisher too.

04. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
$node_subscriber->poll_query_until(
'postgres', q{select (idx_scan=1) from pg_stat_all_indexes where 
indexrelname = 'test_replica_id_full_unique'}
) or die "Timed out while waiting ensuring subscriber used unique index as 
replica identity even with enable_indexscan=false'";
```

03 comment should be added here.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: Allow WindowFuncs prosupport function to use more optimal WindowClause options

[Erwin Brandstetter]

On Wed, 12 Oct 2022 at 05:33, Vik Fearing  wrote:

> On 10/12/22 04:40, David Rowley wrote:
> > I've not really done any analysis into which other window functions
> > can use this optimisation. The attached only adds support to
> > row_number()'s support function and only converts exactly "RANGE
> > UNBOUNDED PRECEDING AND CURRENT ROW" into "ROW UNBOUNDED PRECEDING AND
> > CURRENT ROW".  That might need to be relaxed a little, but I've done
> > no analysis to find that out.
>
> Per spec, the ROW_NUMBER() window function is not even allowed to have a
> frame specified.
>
>  b) The window framing clause of WDX shall not be present.
>
> Also, the specification for ROW_NUMBER() is:
>
>  f) ROW_NUMBER() OVER WNS is equivalent to the :
>
>  COUNT (*) OVER (WNS1 ROWS UNBOUNDED PRECEDING)
>
>
> So I don't think we need to test for anything at all and can
> indiscriminately add or replace the frame with ROWS UNBOUNDED PRECEDING.
>
>
To back this up:
SQL Server returns an error right away if you  try to add a window frame
https://dbfiddle.uk/SplT-F3E

> Msg 10752 Level 15 State 3 Line 1
> The function 'row_number' may not have a window frame.

And Oracle reports a syntax error:
https://dbfiddle.uk/l0Yk8Lw5

row_number() is defined without a "windowing clause" (in Oravle's
nomenclature)
https://docs.oracle.com/cd/B28359_01/server.111/b28286/functions001.htm#i81407
https://docs.oracle.com/cd/B28359_01/server.111/b28286/functions144.htm#i86310

Allowing the same in Postgres (and defaulting to RANGE mode) seems like (a)
genuine bug(s) after all.

Regards
Erwin

Re: Checking pgwin32_is_junction() errors

[Thomas Munro]

On Thu, Aug 11, 2022 at 10:40 PM  wrote:
> On 2022-08-11 07:55, Thomas Munro wrote:
> >> I checked a few variants:
> >>
> >> 21.07.2022  15:11 HOME [\??\Volume{GUID}\]
> >> 09.08.2022  15:06 Test1 [\\?\Volume{GUID}\]
> >> 09.08.2022  15:06 Test2 [\\.\Volume{GUID}\]
> >> 09.08.2022  15:17 Test3 [\??\Volume{GUID}\]
> >> 09.08.2022  15:27 Test4 [C:\temp\1]
> >> 09.08.2022  15:28 Test5 [C:\HOME\Temp\1]
> >
> > One more thing I wondered about, now that we're following junctions
> > outside PGDATA: can a junction point to another junction?  If so, I
> > didn't allow for that: stat() gives up after one hop, because I
> > figured that was enough for the stuff we expect inside PGDATA and I
> > couldn't find any evidence in the man pages that referred to chains.
> > But if you *are* allowed to create a junction "c:\huey" that points to
> > junction "c:\dewey" that points to "c:\louie", and then you do initdb
> > -D c:\huey\pgdata, then I guess it would fail.  Would you mind
> > checking if that is a real possibility, and if so, testing this
> > chain-following patch to see if it fixes it?
>
> I made some junctions and rechecked both patches.
>
> 11.08.2022  16:11 donald [C:\huey]
> 11.08.2022  13:23 huey [C:\dewey]
> 11.08.2022  13:23 dewey [C:\louie]
> 11.08.2022  16:57  louie
>
> With the small attached patch initdb succeeded in any of these
> "directories". If the junction chain is too long, initdb fails with
> "could not create directory" as expected.

Thanks for testing and for that fix!  I do intend to push this, and a
nearby fix for unlink(), but first I want to have test coverage for
all this stuff so we can demonstrate comprehensively that it works via
automated testing, otherwise it's just impossible to maintain (at
least for me, Unix guy).  I have a prototype test suite based on
writing TAP tests in C and I've already found more subtle ancient bugs
around the Windows porting layer... more soon.

Re: PostgreSQL Logical decoding

[Ankit Oza]

Thanks Ashutosh,

Actually we use the Postgres service offered by Azure (Flexible server).
So, I was looking at the following documentation which talks about Logical
Replication and Logical Decoding as two different methods of replication.
Here Logical replication talks about creating both Publisher and Subscriber
settings using simple SQL statements. While for Logical decoding its
talking about publishing WAL but not on how to consume this WAL.
Logical replication and logical decoding - Azure Database for PostgreSQL -
Flexible Server | Microsoft Learn


Also Logical Replication has some limitations like materialized views,
sequences being not replicated. While DDL changes propagation is a common
deficiency among both Logical decoding and Logical Replication. Am I
reading this correctly?
PostgreSQL: Documentation: 12: 30.4. Restrictions


With this reading I thought Logical decoding may be the way to go. However
please guide us on our understanding.

Thanks
Ankit

On Tue, Oct 11, 2022 at 11:01 AM Ashutosh Bapat <
ashutosh.bapat@gmail.com> wrote:

> Hi Ankit,
>
>
> On Tue, Oct 11, 2022 at 9:32 AM Ankit Oza  wrote:
> >
> > Hello,
> >
> > We are looking for an example on how to consume the changes of WAL
> produced by logical decoding (streaming or SQL interface) in another
> postgres server.
>
> built-in logical replication is good example to start looking for.
> https://www.postgresql.org/docs/current/logical-replication.html
>
> >
> > Basically, we are trying to create a replica/standby postgre server to a
> primary progre server. Between Logical replication and Logical Decoding we
> came up with Logical decoding as the choice due to limitation of logical
> replication (materialized views, external views/tables, sequences not
> replicated). However we are not finding a good example with instructions on
> how to set up a consumer postgre server.
> >
>
> Logical decoding is the process to convert WAL to a logical change,
> logical replication deals with transferring these changes to another
> server and applying those there. So they work in tandem; just one
> without the other can not be used. So I am confused about your
> requirements.
>
> --
> Best Wishes,
> Ashutosh Bapat
>

Re: Error "initial slot snapshot too large" in create replication slot

[Michael Paquier]

On Tue, Sep 13, 2022 at 05:31:05PM +0900, Kyotaro Horiguchi wrote:
> And ExportSnapshot repalces oversized subxip with overflowed.
> 
> So even when GetSnapshotData() returns a snapshot with oversized
> subxip, it is saved as just "overflowed" on exporting. I don't think
> this is the expected behavior since such (no xip and overflowed)
> snapshot no longer works.
> 
> On the other hand, it seems to me that snapbuild doesn't like
> takenDuringRecovery snapshots.
> 
> So snapshot needs additional flag signals that xip is oversized and
> all xid are holded there. And also need to let takenDuringRecovery
> suggest subxip is oversized.

The discussion seems to have stopped here.  As this is classified as a
bug fix, I have moved this patch to next CF, waiting on author for the
moment.
--
Michael


signature.asc
Description: PGP signature

Re: collect_corrupt_items_vacuum.patch

[Michael Paquier]

On Wed, Jul 27, 2022 at 09:47:19PM -0400, Robert Haas wrote:
> On Wed, Jul 27, 2022 at 5:56 PM Tom Lane  wrote:
> > Maybe we need a different function for pg_visibility to call?
> > If we want ComputeXidHorizons to serve both these purposes, then it
> > has to always deliver exactly the right answer, which seems like
> > a definition that will be hard and expensive to achieve.
> 
> Yeah, I was thinking along similar lines.
> 
> I'm also kind of wondering why these calculations use
> latestCompletedXid. Is that something we do solely to reduce locking?
> The XIDs of running transactions matter, and their snapshots matter,
> and the XIDs that could start running in the future matter, but I
> don't know why it matters what the latest completed XID is.

Daniel, it seems to me that this thread is waiting for some input from
you, based on the remarks of Tom and Robert.  Are you planning to do
so?  This is marked as a bug fix, so I have moved this item to the
next CF for now.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Fix alter subscription concurrency errors

[Michael Paquier]

On Fri, Sep 09, 2022 at 06:37:07PM +0200, Alvaro Herrera wrote:
> Would it work to use get_object_address() instead?  That would save
> having to write a lookup-and-lock function with a retry loop for each
> object type.

Jeite, this thread is waiting for your input.  This is a bug fix, so I
have moved this patch to the next CF for now to keep track of it.
--
Michael


signature.asc
Description: PGP signature

Re: pg_get_constraintdef: Schema qualify foreign tables unless pretty printing is enabled

[Michael Paquier]

On Wed, Aug 10, 2022 at 09:48:08AM -0400, Tom Lane wrote:
> What I'm inclined to do, rather than repeat the same finicky &
> undocumented coding pattern in one more place, is write a convenience
> function for it that can be named and documented to reflect the coding
> rule about which call sites should use it (rather than calling plain
> generate_relation_name).  However, the first requirement for that
> is to have a clearly defined rule.  I think the intent of 815172ba8068
> was to convert all uses that would determine the object-creation schema
> in commands issued by pg_dump.  Do we want to widen that, and if so
> by how much?  I'd be on board I think with adjusting other ruleutils.c
> functions that could plausibly be used for building creation commands,
> but happen not to be called by pg_dump.  I'm not on board with
> converting every single generate_relation_name call --- mainly because
> it'd be pointless unless you also qualify every single function name,
> operator name, etc; and that would be unreadable.

Lukas, please note that this patch is waiting for your input for a few
weeks now.  Could you reply to the reviews provided?
--
Michael


signature.asc
Description: PGP signature

Re: Fix gin index cost estimation

[Michael Paquier]

On Mon, Sep 19, 2022 at 09:15:25AM +0200, Ronan Dunklau wrote:
> You're right, I was too eager to try to raise the CPU cost proportionnally to 
> the number of array scans (scalararrayop). I'd really like to understand 
> where 
> this equation comes from though... 

So, what's the latest update here?
--
Michael


signature.asc
Description: PGP signature

Re: [BUG] Logical replica crash if there was an error in a function.

[Michael Paquier]

On Sun, Oct 09, 2022 at 12:24:23PM +0300, Anton A. Melnikov wrote:
> On the other hand, function_parse_error_transpose() tries to get
> the original query text  (INSERT INTO test VALUES ('1') in our case) from
> the ActivePortal to clarify the location of the error.
> But in the logrep worker there is no way to restore original query text
> from the logrep message. There is only 'zipped' query equivalent to the 
> original.
> So any function_parse_error_transpose() call seems to be useless
> in the logrep worker.

Yeah, the query string is not available in this context, but it surely
looks wrong to me to assume that something as low-level as
function_parse_error_transpose() needs to be updated for the sake of a
logical worker, while we have other areas that would expect a portal
to be set up.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Clarify the comments about varlena header encoding

[Michael Paquier]

On Mon, Sep 12, 2022 at 11:47:07AM +0700, John Naylor wrote:
> I don't think this clarifies anything, it's just a rephrasing.
> 
> More broadly, I think the description of varlenas in this header is at
> a kind of "local maximum" -- minor adjustments are more likely to make
> it worse. To significantly improve clarity might require a larger
> rewriting, but I'm not personally interested in taking part in that.

This has remained unanswered for four weeks now, so marked as returned
with feedback in the 2022-09 CF.
--
Michael


signature.asc
Description: PGP signature

Re: New Table Access Methods for Multi and Single Inserts

[Michael Paquier]

On Mon, Mar 07, 2022 at 05:09:23PM +0100, Matthias van de Meent wrote:
> That's for the AM-internal flushing; yes. I was thinking about the AM
> api for flushing that's used when finalizing the batched insert; i.e.
> table_multi_insert_flush.
> 
> Currently it assumes that all buffered tuples will be flushed after
> one call (which is correct for heap), but putting those unflushed
> tuples all at once back in memory might not be desirable or possible
> (for e.g. columnar); so we might need to call table_multi_insert_flush
> until there's no more buffered tuples.

This thread has been idle for 6 months now, so I have marked it as
returned with feedback as of what looks like a lack of activity.  I
have looked at what's been proposed, and I am not really sure if the
direction taken is correct, though there may be a potential gain in
consolidating the multi-insert path within the table AM set of
callbacks.
--
Michael


signature.asc
Description: PGP signature

Re: Kerberos delegation support in libpq and postgres_fdw

[Michael Paquier]

On Mon, Sep 19, 2022 at 02:05:39PM -0700, Jacob Champion wrote:
> It's not prevented, because a password is being used. In my tests I'm
> connecting as an unprivileged user.
> 
> You're claiming that the middlebox shouldn't be doing this. If this new
> default behavior were the historical behavior, then I would have agreed.
> But the cat's already out of the bag on that, right? It's safe today.
> And if it's not safe today for some other reason, please share why, and
> maybe I can work on a patch to try to prevent people from doing it.

Please note that this has been marked as returned with feedback in the
current CF, as this has remained unanswered for a bit more than three
weeks.
--
Michael


signature.asc
Description: PGP signature

Re: warn if GUC set to an invalid shared library

[Michael Paquier]

On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> I'm not sure where to go from here.

Not sure either, but the thread has no activity for a bit more than 1
month, so marked as RwF for now.
--
Michael


signature.asc
Description: PGP signature

Re: Add TAP test for auth_delay extension

[Michael Paquier]

On Sat, Jul 30, 2022 at 05:13:12PM -0400, Tom Lane wrote:
> If we don't do it like that, I'd vote for rejecting the patch.

Yep.  Done this way.
--
Michael


signature.asc
Description: PGP signature

Re: New Table Access Methods for Multi and Single Inserts

[Bharath Rupireddy]

On Wed, Oct 12, 2022 at 11:01 AM Michael Paquier  wrote:
>
> This thread has been idle for 6 months now, so I have marked it as
> returned with feedback as of what looks like a lack of activity.  I
> have looked at what's been proposed, and I am not really sure if the
> direction taken is correct, though there may be a potential gain in
> consolidating the multi-insert path within the table AM set of
> callbacks.

Thanks. Unfortunately, I'm not finding enough cycles to work on this
feature. I'm happy to help if others have any further thoughts and
take it from here.

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

Re: pg_checksum: add test for coverage

[Michael Paquier]

On Mon, Aug 29, 2022 at 01:46:25PM +0200, Daniel Gustafsson wrote:
> As written these tests aren't providing more coverage, they run more code but
> they don't ensure that the produced output is correct.  If you write these
> tests with validation on the output they will be a lot more interesting.

DongWook, if you are able to reply back to this feedback, please feel
free to send a new patch.  For now, I have marked this CF entry as
returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: Support for grabbing multiple consecutive values with nextval()

[Michael Paquier]

On Sat, Jul 30, 2022 at 04:21:07PM +0900, Michael Paquier wrote:
> FWIW, I find the result set approach more intuitive and robust,
> particularly in the case of a sequence has non-default values
> INCREMENT and min/max values.  This reduces the dependency of what an
> application needs to know about the details of a given sequence.  With
> only the last value reported, the application would need to compile
> things by itself.

It seems like there is a consensus here, but the thread has no
activity for the past two months, so I have marked the patch as
returned with feedback for now.
--
Michael


signature.asc
Description: PGP signature

Re: remove_useless_groupby_columns is too enthusiastic

[Michael Paquier]

On Fri, Sep 16, 2022 at 12:22:08PM +1200, David Rowley wrote:
> We'd probably still want to keep preprocess_groupclause() as
> get_useful_group_keys_orderings() is not exhaustive in its search.

This thread has been idle for a few weeks now with a review posted, so
I have marked the patch as RwF.
--
Michael


signature.asc
Description: PGP signature

Re: Hash index build performance tweak from sorting

[Michael Paquier]

On Wed, Sep 21, 2022 at 12:43:15PM +0100, Simon Riggs wrote:
> Thanks for tests and review. I'm just jumping on a plane, so may not
> respond in detail until next Mon.

Okay.  If you have time to address that by next CF, that would be
interesting.  For now I have marked the entry as returned with
feedback.
--
Michael


signature.asc
Description: PGP signature

Re: Asynchronous and "direct" IO support for PostgreSQL.

[Michael Paquier]

On Tue, Aug 31, 2021 at 10:56:59PM -0700, Andres Freund wrote:
> I've attached the code for posterity, but the series is large enough that I
> don't think it makes sense to do that all that often... The code is at
> https://github.com/anarazel/postgres/tree/aio

I don't know what's the exact status here, but as there has been no
activity for the past five months, I have just marked the entry as RwF
for now.
--
Michael


signature.asc
Description: PGP signature

Re: Allow placeholders in ALTER ROLE w/o superuser

[Michael Paquier]

On Thu, Jul 21, 2022 at 10:56:41AM -0700, Nathan Bossart wrote:
> Couldn't ProcessGUCArray() use set_config_option_ext() with the context
> indicated by pg_db_role_setting?  Also, instead of using PGC_USERSET in all
> the set_config_option() call sites, shouldn't we remove the "context"
> argument altogether?  I am likely misunderstanding your proposal, but while
> I think simplifying set_config_option() is worthwhile, I don't see why it
> would preclude storing the context in pg_db_role_setting.

This thread has remained idle for a bit more than two months, so I
have marked its CF entry as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Masahiko Sawada]

Hi,

On Tue, Sep 6, 2022 at 3:00 PM Amit Kapila  wrote:
>
> On Mon, Sep 5, 2022 at 5:24 PM Tomas Vondra
>  wrote:
> >
> > On 9/5/22 12:12, Amit Kapila wrote:
> > > On Mon, Sep 5, 2022 at 12:14 PM Tomas Vondra
> > >  wrote:
> > >
> > > It is possible that there is some other problem here that I am
> > > missing. But at this stage, I don't see anything wrong other than the
> > > assertion you have reported.
> > >
> >
> > I'm not sure I agree with that. I'm not convinced the assert is at
> > fault, it might easily be that it hints there's a logic bug somewhere.
> >
>
> It is possible but let's try to prove it. I am also keen to know if
> this hints at a logic bug somewhere.
>
> > >> I think it's mostly clear we won't output this transaction, because the
> > >> restart LSN is half-way through. We can either ignore it at commit time,
> > >> and then we have to make everything work in case we miss assignments (or
> > >> any other part of the transaction).
> > >>
> > >
> > > Note, traditionally, we only form these assignments at commit time
> > > after deciding whether to skip such commits. So, ideally, there
> > > shouldn't be any fundamental problem with not making these
> > > associations before deciding whether we need to replay (send
> > > downstream) any particular transaction.

Agreed.

Summarizing this issue, the assertion check in AssertTXNLsnOrder()
fails as reported because the current logical decoding cannot properly
handle the case where the decoding restarts from NEW_CID. Since we
don't make the association between top-level transaction and its
subtransaction while decoding NEW_CID (ie, in
SnapBuildProcessNewCid()), two transactions are created in
ReorderBuffer as top-txn and have the same LSN. This failure happens
on all supported versions.

To fix the problem, one idea is that we make the association between
top-txn and sub-txn during that by calling ReorderBufferAssignChild(),
as Tomas proposed. On the other hand, since we don't guarantee to make
the association between the top-level transaction and its
sub-transactions until we try to decode the actual contents of the
transaction, it makes sense to me that instead of trying to solve by
making association, we need to change the code which are assuming that
it is associated.

I've attached the patch for this idea. With the patch, we skip the
assertion checks in AssertTXNLsnOrder() until we reach the LSN at
which we start decoding the contents of transaction, ie.
start_decoding_at in SnapBuild. The minor concern is other way that
the assertion check could miss some faulty cases where two unrelated
top-transactions could have same LSN. With this patch, it will pass
for such a case. Therefore, for transactions that we skipped checking,
we do the check when we reach the LSN.

Please note that to pass the new regression tests, the fix proposed in
a related thread[1] is required. Particularly, we need:

@@ -1099,6 +1099,9 @@ SnapBuildCommitTxn(SnapBuild *builder,
XLogRecPtr lsn, TransactionId xid,
else if (sub_needs_timetravel)
{
/* track toplevel txn as well, subxact alone isn't meaningful */
+   elog(DEBUG2, "forced transaction %u to do timetravel
due to one of its subtransaction",
+xid);
+   needs_timetravel = true;
SnapBuildAddCommittedTxn(builder, xid);
}
else if (needs_timetravel)

A side benefit of this approach is that we can fix another assertion
failure too that happens on REL14 and REL15 and reported here[2]. In
the commits 68dcce247f1a(REL14) and 272248a0c1(REL15), the reason why
we make the association between sub-txns to top-txn in
SnapBuildXidSetCatalogChanges() is just to avoid the assertion failure
in AssertTXNLsnOrder(). However, since the invalidation messages are
not transported from sub-txn to top-txn during the assignment, another
assertion check in ReorderBufferForget() fails when forgetting the
subtransaction. If we apply this idea of skipping the assertion
checks, we no longer need to make the such association in
SnapBuildXidSetCatalogChanges() and resolve this issue as well.

> > >>>
> > >>
> > >> Don't we know 848 (the top-level xact) won't be decoded? In that case we
> > >> won't need the snapshot, so why build it?
> > >>
> > >
> > > But this transaction id can be part of committed.xip array if it has
> > > made any catalog changes. We add the transaction/subtransaction to
> > > this array before deciding whether to skip decoding/replay of its
> > > commit.
> > >
> >
> > Hmm, yeah. It's been a while since I last looked into how we build
> > snapshots and how we share them between the transactions :-( If we share
> > the snapshots between transactions, you're probably right we can't just
> > skip these changes.
> >
> > However, doesn't that pretty much mean we *have* to do something about
> > the assignment? I mean, suppose we miss the assignment (like now), so
> > that we end up with two TXNs that we think are top-l

Re: meson PGXS compatibility

[Peter Eisentraut]

On 05.10.22 22:07, Andres Freund wrote:

0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C


I wonder, there has been some work lately to use SIMD and such in other 
places.  Would that affect what kinds of extra CPU-specific compiler 
flags and combinations we might need?


Seems fine otherwise.

0005: meson: Add PGXS compatibility

   The actual meson PGXS compatibility. Plenty more replacements to do, but
   suffices to build common extensions on a few platforms.

   What level of completeness do we want to require here?


I have tried this with a few extensions.  Seems to work alright.  I 
don't think we need to overthink this.  The way it's set up, if someone 
needs additional variables set, they can easily be added.

Re: pg_upgrade failing for 200+ million Large Objects

[Michael Paquier]

On Thu, Sep 08, 2022 at 04:34:07PM -0700, Nathan Bossart wrote:
> On Thu, Sep 08, 2022 at 04:29:10PM -0700, Jacob Champion wrote:
>> To clarify, I agree that pg_dump should contain the core fix. What I'm
>> questioning is the addition of --dump-options to make use of that fix
>> from pg_upgrade, since it also lets the user do "exciting" new things
>> like --exclude-schema and --include-foreign-data and so on. I don't
>> think we should let them do that without a good reason.
> 
> Ah, yes, I think that is a fair point.

It has been more than four weeks since the last activity of this
thread and there has been what looks like some feedback to me, so
marked as RwF for the time being.
--
Michael


signature.asc
Description: PGP signature

Re: Allow pageinspect's bt_page_stats function to return a set of rows instead of a single row

[Michael Paquier]

On Mon, Sep 12, 2022 at 03:18:59PM -0400, Tom Lane wrote:
> I spun up a 32-bit VM, since that had been on my to-do list anyway,
> and it looks like I was right:

This feedback has not been addressed and the thread is idle four
weeks, so I have marked this CF entry as RwF.  Please feel free to
resubmit once a new version of the patch is available.
--
Michael


signature.asc
Description: PGP signature

Re: Temporary file access API

[Michael Paquier]

On Tue, Sep 27, 2022 at 04:22:39PM +, John Morris wrote:
> I’m also wondering about code simplification vs a more general
> encryption/compression framework. I started with the current
> proposal, and I’m also looking at David Steele’s approach to
> encryption/compression in pgbackrest. I’m beginning to think we
> should do a high-level design which includes encryption/compression,
> and then implement it as a “simplification” without actually doing
> the transformations.

It sounds to me like the proposal of this thread is not the most
adapted thing, so I have marked it as RwF as this is the status of 2~3
weeks ago..  If you feel that I am wrong, feel free to scream after
me.
--
Michael


signature.asc
Description: PGP signature

RE: test_decoding assertion failure for the loss of top-sub transaction relationship

[kuroda.hay...@fujitsu.com]

Dear Sawada-san,

Thank you for reviewing HEAD patch! PSA v3 patch.

> +# Test that we can force the top transaction to do timetravel when one of sub
> +# transactions needs that. This is necessary when we restart decoding
> from RUNNING_XACT
> +# without the wal to associate subtransaction to its top transaction.
> 
> I don't think the second sentence is necessary.
> 
> ---
> The last decoding
> +# starts from the first checkpoint and NEW_CID of "s0_truncate"
> doesn't mark the top
> +# transaction as catalog modifying transaction. In this scenario, the
> enforcement sets
> +# needs_timetravel to true even if the top transaction is regarded as
> that it does not
> +# have catalog changes and thus the decoding works without a
> contradition that one
> +# subtransaction needed timetravel while its top transaction didn't.
> 
> I don't understand the last sentence, probably it's a long sentence.
> 
> How about the following description?
> 
> # Test that we can handle the case where only subtransaction is marked
> as containing
> # catalog changes. The last decoding starts from NEW_CID generated by
> "s0_truncate" and
> # marks only the subtransaction as containing catalog changes but we
> don't create the
> # association between top-level transaction and subtransaction yet.
> When decoding the
> # commit record of the top-level transaction, we must force the
> top-level transaction
> # to do timetravel since one of its subtransactions is marked as
> containing catalog changes.

Seems good, I replaced all of comments to yours.

> + elog(DEBUG2, "forced transaction %u to do timetravel due to one of
> its subtransaction",
> + xid);
> + needs_timetravel = true;
> 
> I think "one of its subtransaction" should be "one of its subtransactions".

Fixed. 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



HEAD-v3-0001-Fix-assertion-failure-during-logical-decoding.patch
Description:  HEAD-v3-0001-Fix-assertion-failure-during-logical-decoding.patch

Re: introduce optimized linear search functions that return index of matching element

[John Naylor]

On Sat, Sep 17, 2022 at 12:29 PM Nathan Bossart 
wrote:
>
> On Fri, Sep 16, 2022 at 02:54:14PM +0700, John Naylor wrote:
> > v6 demonstrates why this should have been put off towards the end.
(more below)
>
> Since the SIMD code is fresh in my mind, I wanted to offer my review for
> 0001 in the "Improve dead tuple storage for lazy vacuum" thread [0].
> However, I agree with John that the SIMD part of that work should be left
> for the end

As I mentioned in the radix tree thread, I don't believe this level of
abstraction is appropriate for the intended use case. We'll want to
incorporate some of the low-level simd.h improvements later, so you should
get authorship credit for those.  I've marked the entry "returned with
feedback".

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

Re: Expose Parallelism counters planned/execute in pg_stat_statements

[Michael Paquier]

On Tue, Aug 16, 2022 at 02:58:43PM +0800, Julien Rouhaud wrote:
> I don't think that's an improvement.  With this patch if you see the
> "paral_planned_not_exec" counter going up, you still don't know if this is
> because of the !es_use_parallel_mode or if you simply have too many parallel
> queries running at the same time, or both, and therefore can't do much with
> that information.  Both situations are different and in my opinion require
> different (and specialized) counters to properly handle them.

This thread has been idle for a few weeks now, and this feedback has
not been answered to.  This CF entry has been marked as RwF.
--
Michael


signature.asc
Description: PGP signature

Re: Perform streaming logical transactions by background workers and parallel apply

[Amit Kapila]

On Tue, Oct 11, 2022 at 5:52 AM Masahiko Sawada  wrote:
>
> On Fri, Oct 7, 2022 at 2:00 PM Amit Kapila  wrote:
> >
> > About your point that having different partition structures for
> > publisher and subscriber, I don't know how common it will be once we
> > have DDL replication. Also, the default value of
> > publish_via_partition_root is false which doesn't seem to indicate
> > that this is a quite common case.
>
> So how can we consider these concurrent issues that could happen only
> when streaming = 'parallel'? Can we restrict some use cases to avoid
> the problem or can we have a safeguard against these conflicts?
>

Yeah, right now the strategy is to disallow parallel apply for such
cases as you can see in *0003* patch.

> We
> could find a new problematic scenario in the future and if it happens,
> logical replication gets stuck, it cannot be resolved only by apply
> workers themselves.
>

I think users can change streaming option to on/off and internally the
parallel apply worker can detect and restart to allow replication to
proceed. Having said that, I think that would be a bug in the code and
we should try to fix it. We may need to disable parallel apply in the
problematic case.

The other ideas that occurred to me in this regard are (a) provide a
reloption (say parallel_apply) at table level and we can use that to
bypass various checks like different Unique Key between
publisher/subscriber, constraints/expressions having mutable
functions, Foreign Key (when enabled on subscriber), operations on
Partitioned Table. We can't detect whether those are safe or not
(primarily because of a different structure in publisher and
subscriber) so we prohibit parallel apply but if users use this
option, we can allow it even in those cases. (b) While enabling the
parallel option in the subscription, we can try to match all the
table(s) information of the publisher/subscriber. It will be tricky to
make this work because say even if match some trigger function name,
we won't be able to match the function body. The other thing is when
at a later point the table definition is changed on the subscriber, we
need to again validate the information between publisher and
subscriber which I think would be difficult as we would be already in
between processing some message and getting information from the
publisher at that stage won't be possible.

Thoughts?

-- 
With Regards,
Amit Kapila.

Re: Support logical replication of DDLs

[Dilip Kumar]

On Tue, Oct 11, 2022 at 7:00 PM Ajin Cherian  wrote:
>

I was going through 0001 patch and I have a few comments/suggestions.

1.

@@ -6001,7 +6001,7 @@ getObjectIdentityParts(const ObjectAddress *object,
 transformType = format_type_be_qualified(transform->trftype);
 transformLang = get_language_name(transform->trflang, false);

-appendStringInfo(&buffer, "for %s on language %s",
+appendStringInfo(&buffer, "for %s language %s",
  transformType,
  transformLang);


How is this change related to this patch?

2.
+typedef struct ObjTree
+{
+slist_headparams;
+intnumParams;
+StringInfofmtinfo;
+boolpresent;
+} ObjTree;
+
+typedef struct ObjElem
+{
+char   *name;
+ObjTypeobjtype;
+
+union
+{
+boolboolean;
+char   *string;
+int64integer;
+float8flt;
+ObjTree   *object;
+List   *array;
+} value;
+slist_nodenode;
+} ObjElem;

It would be good to explain these structure members from readability pov.

3.

+
+bool verbose = true;
+

I do not understand the usage of such global variables.  Even if it is
required, add some comments to explain the purpose of it.


4.
+/*
+ * Given a CollectedCommand, return a JSON representation of it.
+ *
+ * The command is expanded fully, so that there are no ambiguities even in the
+ * face of search_path changes.
+ */
+Datum
+ddl_deparse_to_json(PG_FUNCTION_ARGS)
+{

It will be nice to have a test case for this utility function.

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

Re: test_decoding assertion failure for the loss of top-sub transaction relationship

[Masahiko Sawada]

On Wed, Sep 7, 2022 at 11:06 AM kuroda.hay...@fujitsu.com
 wrote:
>
> Dear Amit,
>
> Thanks for giving comments!
>
> > Did you get this new assertion failure after you applied the patch for
> > the first failure? Because otherwise, how can you reach it with the
> > same test case?
>
> The first failure is occurred only in the HEAD, so I did not applied the 
> first patch
> to REL14 and REL15.
> This difference is caused because the commit [Fix catalog lookup...] in 
> REL15(272248a) and older is different
> from the HEAD one.
> In order versions SnapBuildXidSetCatalogChanges() has been added. In the 
> function
> a transaction will be marked as containing catalog changes if the transaction 
> is in InitialRunningXacts,
> and after that the relation between sub-top transactions is assigned based on 
> the parsed->subxact.
> The marking avoids the first failure, but the assignment triggers new failure.
>

FYI, as I just replied to the related thread[1], the assertion failure
in REL14 and REL15 can be fixed by the patch proposed there. So I'd
like to see how the discussion goes. Regardless of this proposed fix,
the patch proposed by Kuroda-san is required for HEAD, REL14, and
REL15, in order to fix the assertion failure in SnapBuildCommitTxn().

Regards,

[1] 
https://www.postgresql.org/message-id/CAD21AoA1gV9pfu8hoXpTQBWH8uEMRg_F_MKM%2BU3Sr0HnyH4AUQ%40mail.gmail.com

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

Re: PostgreSQL Logical decoding

[Amit Kapila]

On Wed, Oct 12, 2022 at 10:09 AM Ankit Oza  wrote:
>
> Thanks Ashutosh,
>
> Actually we use the Postgres service offered by Azure (Flexible server). So, 
> I was looking at the following documentation which talks about Logical 
> Replication and Logical Decoding as two different methods of replication. 
> Here Logical replication talks about creating both Publisher and Subscriber 
> settings using simple SQL statements. While for Logical decoding its talking 
> about publishing WAL but not on how to consume this WAL.
> Logical replication and logical decoding - Azure Database for PostgreSQL - 
> Flexible Server | Microsoft Learn
>
> Also Logical Replication has some limitations like materialized views, 
> sequences being not replicated. While DDL changes propagation is a common 
> deficiency among both Logical decoding and Logical Replication. Am I reading 
> this correctly?
> PostgreSQL: Documentation: 12: 30.4. Restrictions
>
> With this reading I thought Logical decoding may be the way to go. However 
> please guide us on our understanding.
>

Those restrictions (sequences, materialized views, etc.) apply to
logical decoding as well. We don't support decoding operations on
those objects.

-- 
With Regards,
Amit Kapila.

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

[Drouvot, Bertrand]

Hi,

On 10/11/22 8:29 AM, Michael Paquier wrote:

On Mon, Oct 10, 2022 at 09:00:06AM +0200, Drouvot, Bertrand wrote:

foreach(cell, tokens)
{
[...]
+   tokreg = lfirst(cell);
+   if (!token_is_regexp(tokreg))
{
-   if (strcmp(dbname, role) == 0)
+   if (am_walsender && !am_db_walsender)
+   {
+   /*
+* physical replication walsender connections 
can only match
+* replication keyword
+*/
+   if (token_is_keyword(tokreg->authtoken, 
"replication"))
+   return true;
+   }
+   else if (token_is_keyword(tokreg->authtoken, "all"))
return true;


When checking the list of databases in check_db(), physical WAL
senders (aka am_walsender && !am_db_walsender) would be able to accept
regexps, but these should only accept "replication" and never a
regexp, no?  


Oh right, good catch, thanks! Please find attached v6 fixing it.



This is kind of special in the HBA logic, coming back to 9.0 where
physical replication and this special role property have been
introduced.  WAL senders have gained an actual database property later
on in 9.4 with logical decoding, keeping "replication" for
compatibility (connection strings can use replication=database to
connect as a non-physical WAL sender and connect to a specific
database).



Thanks for the explanation!


+typedef struct AuthToken
+{
+   char   *string;
+   boolquoted;
+} AuthToken;
+
+/*
+ * Distinguish the case a token has to be treated as a regular
+ * expression or not.
+ */
+typedef struct AuthTokenOrRegex
+{
+   boolis_regex;
+
+   /*
+* Not an union as we still need the token string for fill_hba_line().
+*/
+   AuthToken  *authtoken;
+   regex_t*regex;
+} AuthTokenOrRegex;


Hmm.  With is_regex to check if a regex_t exists, both structures may
not be necessary. 


Agree that both struct are not necessary. In v6, AuthTokenOrRegex has 
been removed and the regex has been moved to AuthToken. There is no 
is_regex bool anymore, as it's enough to test whether regex is NULL or not.



I have not put my hands on that directly, but if
I guess that I would shape things to have only AuthToken with
(enforcing regex_t in priority if set in the list of elements to check
for a match):
- the string
- quoted
- regex_t
A list member should never have (regex_t != NULL && quoted), right?


The patch does allow that. For example it happens for the test where we 
add a comma in the role name. As we don't rely on a dedicated char to 
mark the end of a reg exp (we only rely on / to mark its start) then 
allowing (regex_t != NULL && quoted) seems reasonable to me.



+# test with a comma in the regular expression
+reset_pg_hba($node, 'all', '"/^.*5,.*e$"', 'password');
+test_conn($node, 'user=md5,role', 'password', 'matching regexp for username',
+   0);


So, we check here that the role includes "5," in its name.  This is
getting fun to parse ;)



Indeed, ;-)



  elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
  {
-   plan skip_all => 'Potentially unsafe test SSL not enabled in 
PG_TEST_EXTRA';
+   plan skip_all =>
+ 'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
  }


Unrelated noise from perltidy.


Right.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comdiff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c6f1b70fd3..406628ef35 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -235,8 +235,9 @@ hostnogssenc  database  
userPostgreSQL database.
-   Multiple database names can be supplied by separating them with
-   commas.  A separate file containing database names can be specified by
+   Multiple database names and/or regular expressions preceded by 
/
+   can be supplied by separating them with commas.
+   A separate file containing database names can be specified by
preceding the file name with @.
   
  
@@ -249,7 +250,8 @@ hostnogssenc  database  
userall specifies that it
matches all users.  Otherwise, this is either the name of a specific
-   database user, or a group name preceded by +.
+   database user, a regular expression preceded by /
+   or a group name preceded by +.
(Recall that there is no real distinction between users and groups
in PostgreSQL; a + mark 
really means
match any of the roles that are directly or indirectly members
@@ -258,7 +260,8 @@ hostnogssenc  database  
user/
+   can be supplied by separating them with commas.
A separate file containing user names can be

Re: future of serial and identity columns

[Corey Huinker]

>
> The feedback was pretty positive, so I dug through all the tests to at
> least get to the point where I could see the end of it.  The attached
> patch 0001 is the actual code and documentation changes.  The 0002 patch
> is just tests randomly updated or disabled to make the whole suite pass.
>   This reveals that there are a few things that would warrant further
> investigation, in particular around extensions and partitioning.  To be
> continued.
>

I like what I see so far!

Question: the xref  refers the reader to sql-createtable, which is a pretty
big page, which could leave the reader lost. Would it make sense to create
a SQL-CREATETABLE-IDENTITY anchor and link to that instead?

RE: test_decoding assertion failure for the loss of top-sub transaction relationship

[kuroda.hay...@fujitsu.com]

Dear Sawada-san,

> FYI, as I just replied to the related thread[1], the assertion failure
> in REL14 and REL15 can be fixed by the patch proposed there. So I'd
> like to see how the discussion goes. Regardless of this proposed fix,
> the patch proposed by Kuroda-san is required for HEAD, REL14, and
> REL15, in order to fix the assertion failure in SnapBuildCommitTxn().

I understood that my patches for REL14 and REL15 might be not needed.
I will check the thread later. Thanks!

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: Tracking last scan time

[Michael Paquier]

On Mon, Oct 03, 2022 at 12:55:40PM +0100, Dave Page wrote:
> Thanks. It's just the changes in xact.c, so it doesn't seem like it would
> cause you any more work either way, in which case, I'll leave it to you :-)

Okay, I have just moved the patch to the next CF then, still marked as
ready for committer.  Are you planning to look at that?
--
Michael


signature.asc
Description: PGP signature

Re: ALTER TABLE uses a bistate but not for toast tables

[Michael Paquier]

On Wed, Sep 07, 2022 at 10:48:39AM +0200, Drouvot, Bertrand wrote:
> +   if (bistate)
> +   {
> +   table_finish_bulk_insert(toastrel, options); // XXX
> 
> I think it's too early, as it looks to me that at this stage we may have not
> finished the whole bulk insert yet.

Yeah, that feels fishy.  Not sure what's the idea behind the XXX
comment, either.  I have marked this patch as RwF, following the lack
of reply.
--
Michael


signature.asc
Description: PGP signature

Re: meson PGXS compatibility

[John Naylor]

On Wed, Oct 12, 2022 at 12:50 PM Peter Eisentraut <
peter.eisentr...@enterprisedb.com> wrote:
>
> On 05.10.22 22:07, Andres Freund wrote:
> > 0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C
>
> I wonder, there has been some work lately to use SIMD and such in other
> places.  Would that affect what kinds of extra CPU-specific compiler
> flags and combinations we might need?

In short, no. The functionality added during this cycle only uses
instructions available by default on the platform. CRC on Arm depends on
armv8-a+crc, and CRC on x86-64 depends on SSE4.2. We can't assume those
currently.

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

Re: Simplify event trigger support checking functions

[Peter Eisentraut]

On 07.10.22 18:43, Robert Haas wrote:

On Fri, Oct 7, 2022 at 8:10 AM Peter Eisentraut
 wrote:

The only drawback in terms of code robustness is that someone adding a
new shared object type would have to remember to add it to
EventTriggerSupportsObjectType().


This doesn't seem like a good idea to me. If the function names give
the idea that you can decide whether new object types should support
event triggers or not, we could change them, or add better comments.
However, right now, you can't add a new object type and fail to update
these functions. With this change, that would become possible. And the
whole point of coding the functions in the way that they are was to
avoid that exact hazard.


I don't think just adding an entry to these functions is enough to make 
event trigger support happen for an object type.  There are other places 
that need changing, and really you need to write a test to check it.  So 
I didn't think these functions provided any actual value.  But I'm not 
too obsessed about it if others feel differently.