Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Magnus Hagander]

On Tue, Dec 20, 2022 at 5:40 AM Michael Paquier  wrote:

> On Tue, Dec 20, 2022 at 09:01:02AM +0900, Michael Paquier wrote:
> > Yeah, my mind was considering as well yesterday the addition of a note
> > in the docs about something among these lines, so fine by me.
>
> And applied that, after tweaking a few tiny things on a last lookup
> with a catversion bump.  Note that the example has been moved at the
> bottom of the table for these functions, which is more consistent with
> the surroundings.
>
>
Hi!

Caught this thread late. To me, pg_dissect_walfile_name() is a really
strange name for a function. Grepping our I code I see the term dissect s
used somewhere inside the regex code and exactly zero instances elsewhere.
Which is why I definitely didn't recognize the term...

Wouldn't something like pg_split_walfile_name() be a lot more consistent
with the rest of our names?

//Magnus

Re: Use get_call_result_type() more widely

[Michael Paquier]

On Mon, Dec 19, 2022 at 07:41:27PM +0530, Bharath Rupireddy wrote:
> I agree with the bucketization. Please see the attached patches. 0001
> - gets rid of explicit tuple desc creation using
> get_call_result_type() for functions thought to be not-so-frequently
> called.

It looks like I am OK with the code paths updated here, which refer to
none of the "critical" function paths.

> 0002 - gets rid of an unnecessary call to BlessTupleDesc()
> after get_call_result_type().

Hmm.  I am not sure whether this is right, actually..
--
Michael


signature.asc
Description: PGP signature

Re: Use get_call_result_type() more widely

[Michael Paquier]

On Mon, Dec 19, 2022 at 05:50:03PM -0500, Robert Haas wrote:
> All right, well, I just work here. :-)

Just to give some numbers.  The original version of the patch doing
the full switch removed 500 lines of code.  The second version that
switches the "non-critical" paths removes 200~ lines.
--
Michael


signature.asc
Description: PGP signature

Re: Refactor SCRAM code to dynamically handle hash type and key length

[Michael Paquier]

On Tue, Dec 20, 2022 at 08:58:38AM +0900, Michael Paquier wrote:
> Thanks!  I have applied for I have here..  There are other pieces to
> think about in this area.

FYI, I have spent a few hours looking at the remaining parts of the
SCRAM code that could be simplified if a new hash method is added, and
this b3bb7d1 has really made things easier.  There are a few things
that will need more thoughts.  Here are my notes, assuming that
SHA-512 is done:
1) HBA entries had better use a new keyword for scram-sha-512, implying
a new uaSCRAM512 to combine with the existing uaSCRAM.  One reason
behind that it to advertise the mechanisms supported back to the
client depending on the matching HBA entry.
2) If a role has a SCRAM-SHA-256 password and the HBA entry matches
scram-sha-512, the SASL exchange needs to go through the mock process
with SHA-512 and fail.
3) If a role has a SCRAM-SHA-512 password and the HBA entry matches
scram-sha-256, the SASL exchange needs to go through the mock process
with SHA-256 and fail.
4) The case of MD5 is something that looks a bit tricky at quick
glance.  We know that if the role has a MD5 password stored, we will
fail anyway.  So we could just advertise the SHA-256 mechanisms in
this case and map the mock to that?
5) The mechanism choice in libpq needs to be reworked a bit based on
what the backend sends.  There may be no point in advertising all the
SHA-256 and SHA-512 mechanisms at the same time, I guess.

Attached is a WIP patch that I have played with.  This shows the parts
of the code that would need more thoughts if implementing such
things.  This works for the cases 1~3 (see the TAP tests).  I've given
up on the MD5 case 4 for now, but perhaps I just missed a simple trick.
5 in libpq uses dirty tricks.  I have marked this CF entry as
committed, and I'll come back to each relevant part on new separate
threads.
--
Michael
From 49a3c993a1df722da01f16839eb1d6185c205092 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 20 Dec 2022 16:14:41 +0900
Subject: [PATCH] WIP SCRAM-SHA-512

---
 src/include/common/scram-common.h |   5 +-
 src/include/libpq/crypt.h |   3 +-
 src/include/libpq/hba.h   |   3 +-
 src/include/libpq/scram.h |   3 +-
 src/backend/libpq/auth-scram.c| 143 ++
 src/backend/libpq/auth.c  |  15 ++-
 src/backend/libpq/crypt.c |  13 +-
 src/backend/libpq/hba.c   |   5 +-
 src/backend/utils/misc/guc_tables.c   |   1 +
 src/common/scram-common.c |  13 +-
 src/interfaces/libpq/fe-auth-scram.c  |  24 +++-
 src/interfaces/libpq/fe-auth.c|  52 +---
 src/interfaces/libpq/fe-auth.h|   2 +
 src/test/authentication/t/001_password.pl |  21 +++-
 14 files changed, 236 insertions(+), 67 deletions(-)

diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 953d30ac54..10996fa735 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -19,15 +19,18 @@
 /* Name of SCRAM mechanisms per IANA */
 #define SCRAM_SHA_256_NAME "SCRAM-SHA-256"
 #define SCRAM_SHA_256_PLUS_NAME "SCRAM-SHA-256-PLUS"	/* with channel binding */
+#define SCRAM_SHA_512_NAME "SCRAM-SHA-512"
+#define SCRAM_SHA_512_PLUS_NAME "SCRAM-SHA-512-PLUS"	/* with channel binding */
 
 /* Length of SCRAM keys (client and server) */
 #define SCRAM_SHA_256_KEY_LENPG_SHA256_DIGEST_LENGTH
+#define SCRAM_SHA_512_KEY_LENPG_SHA512_DIGEST_LENGTH
 
 /*
  * Size of buffers used internally by SCRAM routines, that should be the
  * maximum of SCRAM_SHA_*_KEY_LEN among the hash methods supported.
  */
-#define SCRAM_MAX_KEY_LEN	SCRAM_SHA_256_KEY_LEN
+#define SCRAM_MAX_KEY_LEN	SCRAM_SHA_512_KEY_LEN
 
 /*
  * Size of random nonce generated in the authentication exchange.  This
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 3238cf66d3..992d4922fc 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -28,7 +28,8 @@ typedef enum PasswordType
 {
 	PASSWORD_TYPE_PLAINTEXT = 0,
 	PASSWORD_TYPE_MD5,
-	PASSWORD_TYPE_SCRAM_SHA_256
+	PASSWORD_TYPE_SCRAM_SHA_256,
+	PASSWORD_TYPE_SCRAM_SHA_512
 } PasswordType;
 
 extern PasswordType get_password_type(const char *shadow_pass);
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 90c51ad6fa..dc536225ef 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -30,7 +30,8 @@ typedef enum UserAuth
 	uaIdent,
 	uaPassword,
 	uaMD5,
-	uaSCRAM,
+	uaSCRAM256,
+	uaSCRAM512,
 	uaGSS,
 	uaSSPI,
 	uaPAM,
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index b29501ef96..f2aa5c82c2 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -22,7 +22,8 @@
 extern PGDLLIMPORT const pg_be_sasl_mech pg_be_scram_mech;
 
 /* Routines to handle and check SCRAM-SHA-256 secret */
-extern char *pg_be_scram_build_secret(const char *password);
+extern 

Re: code cleanups

[John Naylor]

On Thu, Nov 24, 2022 at 12:53 AM Tom Lane  wrote:
>
> Justin Pryzby  writes:
> > Some modest cleanups I've accumulated.

> 0004: Right, somebody injected code in a poorly chosen place
> (yet another victim of the "add at the end" anti-pattern).

I've pushed 0004.

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

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

[Peter Smith]

On Tue, Dec 20, 2022 at 2:20 PM Amit Kapila  wrote:
>
> On Tue, Dec 20, 2022 at 8:17 AM Peter Smith  wrote:
> >
> > Summary
> > ---
> >
> > In summary, everything I have tested so far appeared to be working
> > properly. In other words, for overlapping streamed transactions of
> > different kinds, and regardless of whether zero/some/all of those
> > transactions are getting processed by a PA worker, the resulting
> > replicated data looked consistently OK.
> >
>
> Thanks for doing the detailed testing of this patch. I think the one
> area where we can focus more is the switch-to-serialization mode while
> sending changes to the parallel worker.
>
> >
> > NOTE - all testing described in this post above was using v58-0001
> > only. However, the point of implementing these as a .spec test was to
> > be able to repeat these same regression tests on newer versions with
> > minimal manual steps required. Later I plan to fetch/apply the most
> > recent patch version and repeat these same tests.
> >
>
> That would be really helpful.
>

FYI, my pub-sub.spec tests gave the same result (i.e. pass) when
re-run against the latest v62-0001 (parallel apply base patch) and
v62-0004 (GUC 'force_stream_mode' patch).

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: [PoC] Improve dead tuple storage for lazy vacuum

[John Naylor]

On Mon, Dec 19, 2022 at 2:14 PM Masahiko Sawada 
wrote:
>
> On Tue, Dec 13, 2022 at 1:04 AM Masahiko Sawada 
wrote:

> > Looking at other code using DSA such as tidbitmap.c and nodeHash.c, it
> > seems that they look at only memory that are actually dsa_allocate'd.
> > To be exact, we estimate the number of hash buckets based on work_mem
> > (and hash_mem_multiplier) and use it as the upper limit. So I've
> > confirmed that the result of dsa_get_total_size() could exceed the
> > limit. I'm not sure it's a known and legitimate usage. If we can
> > follow such usage, we can probably track how much dsa_allocate'd
> > memory is used in the radix tree.
>
> I've experimented with this idea. The newly added 0008 patch changes
> the radix tree so that it counts the memory usage for both local and
> shared cases. As shown below, there is an overhead for that:
>
> w/o 0008 patch
>  298453544 | 282

> w/0 0008 patch
>  293603184 | 297

This adds about as much overhead as the improvement I measured in the v4
slab allocator patch. That's not acceptable, and is exactly what Andres
warned about in

https://www.postgresql.org/message-id/20220704211822.kfxtzpcdmslzm2dy%40awork3.anarazel.de

I'm guessing the hash join case can afford to be precise about memory
because it must spill to disk when exceeding workmem. We don't have that
design constraint.

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

Re: Common function for percent placeholder replacement

[Peter Eisentraut]

On 19.12.22 10:51, Alvaro Herrera wrote:

I think the new one is not great.  I wish we could do something more
straightforward, maybe like

   replace_percent_placeholders(base_command,
PERCENT_OPT("f", filename),
PERCENT_OPT("d", target_detail));

Is there a performance disadvantage to a variadic implementation?
Alternatively, have all these macro calls form an array.


How about this new one with variable arguments?

(Still need to think about Robert's comment about lack of error context.)
From 76dbfd291313e610aadb0e01eb46f1b0113f2c0f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 20 Dec 2022 06:27:42 +0100
Subject: [PATCH v3] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.

Discussion: 
https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com
---
 .../basebackup_to_shell/basebackup_to_shell.c |  55 +---
 src/backend/access/transam/xlogarchive.c  |  45 +--
 src/backend/libpq/be-secure-common.c  |  38 ++
 src/backend/postmaster/shell_archive.c|  58 ++---
 src/common/Makefile   |   1 +
 src/common/archive.c  |  81 +---
 src/common/meson.build|   1 +
 src/common/percentrepl.c  | 123 ++
 src/include/common/percentrepl.h  |  18 +++
 9 files changed, 175 insertions(+), 245 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c 
b/contrib/basebackup_to_shell/basebackup_to_shell.c
index bdaf67a4c8..0dceab65f8 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,7 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
const char *target_detail)
 {
-   StringInfoData buf;
-   const char *c;
-
-   initStringInfo();
-   for (c = base_command; *c != '\0'; ++c)
-   {
-   /* Anything other than '%' is copied verbatim. */
-   if (*c != '%')
-   {
-   appendStringInfoChar(, *c);
-   continue;
-   }
-
-   /* Any time we see '%' we eat the following character as well. 
*/
-   ++c;
-
-   /*
-* The following character determines what we insert here, or 
may
-* cause us to throw an error.
-*/
-   if (*c == '%')
-   {
-   /* '%%' is replaced by a single '%' */
-   appendStringInfoChar(, '%');
-   }
-   else if (*c == 'f')
-   {
-   /* '%f' is replaced by the filename */
-   appendStringInfoString(, filename);
-   }
-   else if (*c == 'd')
-   {
-   /* '%d' is replaced by the target detail */
-   appendStringInfoString(, target_detail);
-   }
-   else if (*c == '\0')
-   {
-   /* Incomplete escape sequence, expected a character 
afterward */
-   ereport(ERROR,
-   errcode(ERRCODE_SYNTAX_ERROR),
-   errmsg("shell command ends unexpectedly 
after escape character \"%%\""));
-   }
-   else
-   {
-   /* Unknown escape sequence */
-   ereport(ERROR,
-   errcode(ERRCODE_SYNTAX_ERROR),
-   errmsg("shell command contains 
unexpected escape sequence \"%c\"",
-  *c));
-   }
-   }
-
-   return buf.data;
+   return replace_percent_placeholders(base_command, "df", target_detail, 
filename);
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c 
b/src/backend/access/transam/xlogarchive.c
index e2b7176f2f..7b32f67331 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -23,6 +23,7 @@
 #include "access/xlog_internal.h"
 #include 

Re: meson files copyright

[Noah Misch]

On Mon, Dec 19, 2022 at 09:09:25PM +0100, Peter Eisentraut wrote:
> On 19.12.22 19:33, Robert Haas wrote:
> >On Mon, Dec 19, 2022 at 1:03 PM Tom Lane  wrote:
> >>Vik Fearing  writes:
> >>>Perhaps a bit off-topic, but what is the point of the file identifiers?
> >>
> >>IMO, it helps to tell things apart when you've got a bunch of editor
> >>windows open on some mighty samey-looking meson.build files.
> >
> >On the other hand, maintaining those identification lines in all of
> >our files has a pretty high distributed cost. I never use them to
> >figure out what file I'm editing because my editor can tell me that.
> >But I do have to keep fixing those lines as I create new files. It's
> >not the most annoying thing ever, but I wouldn't mind a bit if I
> >didn't have to do it any more.
> 
> I agree it's not very useful and a bit annoying.

Agreed.  To me, it's just one more thing to get wrong.

Re: Force streaming every change in logical decoding

[Dilip Kumar]

On Wed, Dec 14, 2022 at 5:29 PM Amit Kapila  wrote:
>
> On Wed, Dec 14, 2022 at 2:15 PM shiy.f...@fujitsu.com
>  wrote:
> >
> > Please see the attached patch. I also fix Peter's comments[1]. The GUC name 
> > and
> > design are still under discussion, so I didn't modify them.
> >
>
> Let me summarize the discussion on name and design till now. As per my
> understanding, we have three requirements: (a) In publisher, stream
> each change of transaction instead of waiting till
> logical_decoding_work_mem or commit; this will help us to reduce the
> test timings of current and future tests for replication of
> in-progress transactions; (b) In publisher, serialize each change
> instead of waiting till logical_decoding_work_mem; this can help
> reduce the test time of tests related to serialization of changes in
> logical decoding; (c) In subscriber, during parallel apply for
> in-progress transactions (a new feature being discussed at [1]) allow
> the system to switch to serialize mode (no more space in shared memory
> queue between leader and parallel apply worker either due to a
> parallel worker being busy or waiting on some lock) while sending
> changes.
>
> Having a GUC that controls these actions/features will allow us to
> write tests with shorter duration and better predictability as
> otherwise, they require a lot of changes. Apart from tests, these also
> help to easily debug the required code. So they fit the Developer
> Options category of GUC [2].
>
> We have discussed three different ways to provide GUC for these
> features. (1) Have separate GUCs like force_server_stream_mode,
> force_server_serialize_mode, force_client_serialize_mode (we can use
> different names for these) for each of these; (2) Have two sets of
> GUCs for server and client. We can have logical_decoding_mode with
> values as 'stream' and 'serialize' for the server and then
> logical_apply_serialize = true/false for the client. (3) Have one GUC
> like logical_replication_mode with values as 'server_stream',
> 'server_serialize', 'client_serialize'.
>
> The names used here are tentative mainly to explain each of the
> options, we can use different names once we decide among the above.
>
> Thoughts?

I think option 2 makes sense because stream/serialize are two related
options and both are dependent on the same parameter
(logical_decoding_work_mem) so having a single know is logical.  And
another GUC for serializing logical apply.

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

Re: Force streaming every change in logical decoding

[Dilip Kumar]

On Wed, Dec 14, 2022 at 2:15 PM shiy.f...@fujitsu.com
 wrote:

> > -while (rb->size >= logical_decoding_work_mem * 1024L)
> > +while ((!force_stream && rb->size >= logical_decoding_work_mem *
> > 1024L) ||
> > +   (force_stream && rb->size > 0))
> >  {
> >
> > It seems like if force_stream is on then indirectly it is enabling
> > force serialization as well.  Because once we enter into the loop
> > based on "force_stream" then it will either stream or serialize but I
> > guess we do not want to force serialize based on this parameter.
> >
>
> Agreed, I refactored the code and modified this point.

After thinking more on this I feel the previous behavior made more
sense.  Because without this patch if we cross the work_mem we try to
stream and if we can not stream for some reason e.g. partial change
then we serialize.  And I feel your previous patch was mimicking the
same behavior for each change.  Now in the new patch, we will try to
stream and if we can not we will queue the change so I feel we are
creating a new patch that actually doesn't exist without the force
mode.

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

Re: Time delayed LR (WAS Re: logical replication restrictions)

[Amit Kapila]

On Fri, Dec 16, 2022 at 12:11 PM Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Amit,
>
> > I also don't see the need for this mechanism for logical replication,
> > and in fact, why do we need to even wait for sending the existing WAL?
>
> Is it meant that logicalrep walsenders do not have to track WalSndCaughtUp and
> any pending data in the output buffer?
>

I haven't checked the details but I think what you are saying is correct.

>
> > Another related point to consider is what is the behavior of
> > synchronous replication when shutdown has been performed both in the
> > case of physical and logical replication especially when the
> > time-delayed replication feature is enabled?
>
> In physical replication without any failures, it seems that users can stop 
> primary
> server even if the applications are delaying on secondary. This is because 
> sent WALs
> are immediately flushed on secondary and walreceiver replies its position.
>

What happens when synchronous_commit's value is remote_apply and the
user has also set synchronous_standby_names to corresponding standby?

-- 
With Regards,
Amit Kapila.

Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Michael Paquier]

On Tue, Dec 20, 2022 at 09:01:02AM +0900, Michael Paquier wrote:
> Yeah, my mind was considering as well yesterday the addition of a note
> in the docs about something among these lines, so fine by me.

And applied that, after tweaking a few tiny things on a last lookup
with a catversion bump.  Note that the example has been moved at the
bottom of the table for these functions, which is more consistent with
the surroundings. 
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Doc: Explain about Column List feature.

[Peter Smith]

On Tue, Dec 20, 2022 at 3:47 AM Alvaro Herrera  wrote:
>
> On 2022-Sep-15, Alvaro Herrera wrote:
>
> > On 2022-Sep-15, Alvaro Herrera wrote:
> >
> > > Looking at the rendered docs again, I notice that section "31.4.5.
> > > Combining Multiple Column Lists" is *only* the red-tinted Warning block.
> > > That seems quite odd.  I am tempted to remove the sect2 heading for that
> > > one too.
> >
> > Pushed.  I didn't modify this part; I spent too much time looking at it
> > trying to figure out how to do it.  I think this bit really belongs in
> > the CREATE/ALTER docs rather than this chapter.  But in order to support
> > having a separate  for the restriction on combination, we need a
> > separate  for the column_name parameter. So I'm going to
> > edit that one and I'll throw this change in.
>
> I figured out how to fix this one -- just remove the  tags, and
> add a  tag to the  box.  The attached yields the
> explanatory text in a separate box that doesn't have the silly
> otherwise-empty section title.  We add the 'id' that was in the sect2 to
> the warning; with this, at the referencing end the full title is
> rendered, which looks quite reasonable.  I have attached screenshots of
> both sides of this.
>
> Compare the existing
> https://www.postgresql.org/docs/15/logical-replication-col-lists.html#LOGICAL-REPLICATION-COL-LIST-COMBINING
>
> Unless there are objections, I'll get this pushed to 15 and master.
>

Not quite an objection, but...

If you change this warning title then it becomes the odd one out -
every other warning in all the pg docs just says "Warning".  IMO
maintaining consistency throughout is best. e.g. I can imagine maybe
someone searching for "Warning" in the docs, and now they are not
going to find this one.

Maybe a safer way to fix this "silly otherwise-empty section title"
would be to just add some explanatory text so it is no longer empty.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Add sub-transaction overflow status in pg_stat_activity

[Dilip Kumar]

On Tue, Dec 20, 2022 at 2:32 AM Robert Haas  wrote:
>
> On Mon, Dec 19, 2022 at 3:48 PM Ted Yu  wrote:
> > It seems the comment for `backend_subxact_overflowed` missed a word.
> >
> > Please see the patch.
>
> Committed this fix, thanks.

Thanks, Robert!

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

Re: pgsql: Doc: Explain about Column List feature.

[Amit Kapila]

On Mon, Dec 19, 2022 at 10:17 PM Alvaro Herrera  wrote:
>
> On 2022-Sep-15, Alvaro Herrera wrote:
>
> > On 2022-Sep-15, Alvaro Herrera wrote:
> >
> > > Looking at the rendered docs again, I notice that section "31.4.5.
> > > Combining Multiple Column Lists" is *only* the red-tinted Warning block.
> > > That seems quite odd.  I am tempted to remove the sect2 heading for that
> > > one too.
> >
> > Pushed.  I didn't modify this part; I spent too much time looking at it
> > trying to figure out how to do it.  I think this bit really belongs in
> > the CREATE/ALTER docs rather than this chapter.  But in order to support
> > having a separate  for the restriction on combination, we need a
> > separate  for the column_name parameter. So I'm going to
> > edit that one and I'll throw this change in.
>
> I figured out how to fix this one -- just remove the  tags, and
> add a  tag to the  box.  The attached yields the
> explanatory text in a separate box that doesn't have the silly
> otherwise-empty section title.  We add the 'id' that was in the sect2 to
> the warning; with this, at the referencing end the full title is
> rendered, which looks quite reasonable.  I have attached screenshots of
> both sides of this.
>
> Compare the existing
> https://www.postgresql.org/docs/15/logical-replication-col-lists.html#LOGICAL-REPLICATION-COL-LIST-COMBINING
>

-  
-   Combining Multiple Column Lists
-
-   
+   
+Combining Column Lists from Multiple Subscriptions

Shouldn't the title be "Combining Column Lists from Multiple
Publications"? We can define column lists while defining publications
so the proposed title doesn't seem to be conveying the right thing.

-- 
With Regards,
Amit Kapila.

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

[Amit Kapila]

On Tue, Dec 20, 2022 at 8:17 AM Peter Smith  wrote:
>
> Summary
> ---
>
> In summary, everything I have tested so far appeared to be working
> properly. In other words, for overlapping streamed transactions of
> different kinds, and regardless of whether zero/some/all of those
> transactions are getting processed by a PA worker, the resulting
> replicated data looked consistently OK.
>

Thanks for doing the detailed testing of this patch. I think the one
area where we can focus more is the switch-to-serialization mode while
sending changes to the parallel worker.

>
> NOTE - all testing described in this post above was using v58-0001
> only. However, the point of implementing these as a .spec test was to
> be able to repeat these same regression tests on newer versions with
> minimal manual steps required. Later I plan to fetch/apply the most
> recent patch version and repeat these same tests.
>

That would be really helpful.

-- 
With Regards,
Amit Kapila.

Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Michael Paquier]

On Mon, Dec 19, 2022 at 05:51:19PM +0530, Bharath Rupireddy wrote:
> A nitpick - can we also specify a use case for the function
> pg_dissect_walfile_name(), that is, computing LSN from offset and WAL
> file name, something like [1]?

Yeah, my mind was considering as well yesterday the addition of a note
in the docs about something among these lines, so fine by me.
--
Michael


signature.asc
Description: PGP signature

Re: Refactor SCRAM code to dynamically handle hash type and key length

[Michael Paquier]

On Mon, Dec 19, 2022 at 02:58:24PM -0500, Jonathan S. Katz wrote:
> With the assertion in "scram_build_secret", that value is set from the
> "PG_SHA256" constant anyway, so I don't know if it actually gives us
> anything other than a reminder? With "scram_mock"salt" the value ultimately
> comes from state (which is currently set from the constant), so perhaps
> there is a guard there.

Yes, these mostly act as reminders to anybody touching this code, so
I'd like to keep both.  For the mock part, we may also want to use
something different than SHA-256.

> At a minimum, I'd suggest a comment around it, especially if it's set up to
> be removed at a future date.

Okay, sure.

> - I do like the "SCRAM_MAX_KEY_LEN" change, and I see we're now passing
> "key_length" around to ensure we're only using the desired number of bytes.
> I am a little queasy that once we expand "SCRAM_MAX_KEY_LEN" we run the risk
> of having the smaller hashes accidentally use the extra bytes in their
> calculations. However, I think that's more a fear than not, an we can
> mitigate the risk with testing.

A few code paths relied on the size of these local buffers, now they
just use the passed-in key length from the state.

> No objections. I think this decreases the lift to supporting more variations
> of SCRAM.
> 
> Once committed, I'll rebase the server-side SCRAM functions patch with this.
> I may want to rethink the interface for that to allow the digest to be
> "selectable" (vs. from the function) but I'll discuss on that thread[1].

Thanks!  I have applied for I have here..  There are other pieces to
think about in this area.
--
Michael


signature.asc
Description: PGP signature

CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Corey Huinker]

Attached is my work in progress to implement the changes to the CAST()
function as proposed by Vik Fearing.

This work builds upon the Error-safe User Functions work currently ongoing.

The proposed changes are as follows:

CAST(expr AS typename)
continues to behave as before.

CAST(expr AS typename ERROR ON ERROR)
has the identical behavior as the unadorned CAST() above.

CAST(expr AS typename NULL ON ERROR)
will use error-safe functions to do the cast of expr, and will return
NULL if the cast fails.

CAST(expr AS typename DEFAULT expr2 ON ERROR)
will use error-safe functions to do the cast of expr, and will return
expr2 if the cast fails.

There is an additional FORMAT parameter that I have not yet implemented, my
understanding is that it is largely intended for DATE/TIME field
conversions, but others are certainly possible.
CAST(expr AS typename FORMAT fmt DEFAULT expr2 ON ERROR)

What is currently working:
- Any scalar expression that can be evaluated at parse time. These tests
from cast.sql all currently work:

VALUES (CAST('error' AS integer));
VALUES (CAST('error' AS integer ERROR ON ERROR));
VALUES (CAST('error' AS integer NULL ON ERROR));
VALUES (CAST('error' AS integer DEFAULT 42 ON ERROR));

SELECT CAST('{123,abc,456}' AS integer[] DEFAULT '{-789}' ON ERROR) as
array_test1;

- Scalar values evaluated at runtime.

CREATE TEMPORARY TABLE t(t text);
INSERT INTO t VALUES ('a'), ('1'), ('b'), (2);
SELECT CAST(t.t AS integer DEFAULT -1 ON ERROR) AS foo FROM t;
 foo
-
  -1
   1
  -1
   2
(4 rows)


Along the way, I made a few design decisions, each of which is up for
debate:

First, I created OidInputFunctionCallSafe, which is to OidInputFunctionCall
what InputFunctionCallSafe is to InputFunctionCall. Given that the only
place I ended up using it was stringTypeDatumSafe(), it may be possible to
just move that code inside stringTypeDatumSafe.

Next, I had a need for FuncExpr, CoerceViaIO, and ArrayCoerce to all report
if their expr argument failed, and if not, just past the evaluation of
expr2. Rather than duplicate this logic in several places, I chose instead
to modify CoalesceExpr to allow for an error-test mode in addition to its
default null-test mode, and then to provide this altered node with two
expressions, the first being the error-safe typecast of expr and the second
being the non-error-safe typecast of expr2.

I still don't have array-to-array casts working, as the changed I would
likely need to make to ArrayCoerce get somewhat invasive, so this seemed
like a good time to post my work so far and solicit some feedback beyond
what I've already been getting from Jeff Davis and Michael Paquier.

I've sidestepped domains as well for the time being as well as avoiding JIT
issues entirely.

No documentation is currently prepared. All but one of the regression test
queries work, the one that is currently failing is:

SELECT CAST('{234,def,567}'::text[] AS integer[] DEFAULT '{-1011}' ON
ERROR) as array_test2;

Other quirks:
- an unaliased CAST ON DEFAULT will return the column name of "coalesce",
which internally is true, but obviously would be quite confusing to a user.

As a side observation, I noticed that the optimizer already tries to
resolve expressions based on constants and to collapse expression trees
where possible, which makes me wonder if the work done to do the same in
transformTypeCast/ and coerce_to_target_type and coerce_type isn't also
wasted.
From 897c9c68a29ad0fa57f28734df0c77553e026d80 Mon Sep 17 00:00:00 2001
From: coreyhuinker 
Date: Sun, 18 Dec 2022 16:20:01 -0500
Subject: [PATCH 1/3] add OidInputFunctionCall

---
 src/backend/utils/fmgr/fmgr.c | 13 +
 src/include/fmgr.h|  4 
 2 files changed, 17 insertions(+)

diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 0d37f69298..e9a19ce653 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1701,6 +1701,19 @@ OidInputFunctionCall(Oid functionId, char *str, Oid typioparam, int32 typmod)
 	return InputFunctionCall(, str, typioparam, typmod);
 }
 
+bool
+OidInputFunctionCallSafe(Oid functionId, char *str, Oid typioparam,
+		 int32 typmod, fmNodePtr escontext,
+		 Datum *result)
+{
+	FmgrInfo			flinfo;
+
+	fmgr_info(functionId, );
+
+	return InputFunctionCallSafe(, str, typioparam, typmod,
+ escontext, result);
+}
+
 char *
 OidOutputFunctionCall(Oid functionId, Datum val)
 {
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index b7832d0aa2..b835ef72b5 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -706,6 +706,10 @@ extern bool InputFunctionCallSafe(FmgrInfo *flinfo, char *str,
   Datum *result);
 extern Datum OidInputFunctionCall(Oid functionId, char *str,
   Oid typioparam, int32 typmod);
+extern bool
+OidInputFunctionCallSafe(Oid functionId, char *str, Oid typioparam,
+		 int32 typmod, fmNodePtr escontext,
+		 Datum *result);
 extern char *OutputFunctionCall(FmgrInfo 

Re: Use get_call_result_type() more widely

[Robert Haas]

On Mon, Dec 19, 2022 at 4:21 PM Tom Lane  wrote:
> Now that somebody's made an effort to identify which places are
> potentially performance-critical, I don't see why we wouldn't use
> the fruits of their labor.  Yes, somebody else might draw the line
> differently, but drawing a line at all seems like a step forward
> to me.

All right, well, I just work here. :-)

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

Re: Error-safe user functions

[Robert Haas]

On Mon, Dec 19, 2022 at 4:27 PM Tom Lane  wrote:
> In [1] I wrote
>
> >>> I'm a little concerned about the cost-benefit of fixing the reg* types.
> >>> The ones that accept type names actually use the core grammar to parse
> >>> those.  Now, we probably could fix the grammar to be non-throwing, but
> >>> it'd be very invasive and I'm not sure about the performance impact.
> >>> It might be best to content ourselves with soft reporting of lookup
> >>> failures, as opposed to syntax problems.

Ah right.  I agree that invading the main grammar doesn't seem
terribly appealing. Setting regtypein aside could be a sensible
choice, then. Another option might be to have some way of parsing type
names outside of the main grammar, which would be more work and would
require keeping things in sync, but perhaps it would end up being less
ugly

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

Re: appendBinaryStringInfo stuff

[David Rowley]

On Tue, 20 Dec 2022 at 11:42, Tom Lane  wrote:
> I think Peter is entirely right to question whether *this* type's
> output function is performance-critical.  Who's got large tables with
> jsonpath columns?  It seems to me the type would mostly only exist
> as constants within queries.

The patch touches code in the path of jsonb's output function too. I
don't think you could claim the same for that.

David

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

[Jeff Davis]

On Sun, 2022-12-18 at 17:38 -0600, Justin Pryzby wrote:
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +   List   *ancestors = get_partition_ancestors(relid);
> > +   Oid root = InvalidOid;
> > 
> > nit: it would be better if the variable `root` can be aligned with
> > variable
> > `ancestors`.
> 
> It is aligned, but only after configuring one's editor/pager/mail
> client
> to display tabs in the manner assumed by postgres' coding style.

If you use emacs or vim, there are editor config samples in
src/tools/editors/

Regards,
Jeff Davis

Re: appendBinaryStringInfo stuff

[Tom Lane]

David Rowley  writes:
> On Tue, 20 Dec 2022 at 09:23, Peter Eisentraut
>  wrote:
>> AFAICT, the code in question is for the text output of the jsonpath
>> type, which is used ... for barely anything?

> I think the performance of a type's output function is quite critical.

I think Peter is entirely right to question whether *this* type's
output function is performance-critical.  Who's got large tables with
jsonpath columns?  It seems to me the type would mostly only exist
as constants within queries.

regards, tom lane

Re: appendBinaryStringInfo stuff

[David Rowley]

On Tue, 20 Dec 2022 at 09:23, Peter Eisentraut
 wrote:
> AFAICT, the code in question is for the text output of the jsonpath
> type, which is used ... for barely anything?

I think the performance of a type's output function is quite critical.
I've seen huge performance gains in COPY TO performance from
optimising output functions in the past (see dad75eb4a and aa2387e2f).
It would be good to see some measurements to find out how much adding
the strlen calls back in would cost us. If we're unable to measure the
change, then maybe the cleanup patch would be nice. If it's going to
slow COPY TO down 10-20%, then we need to leave this or consider the
inline function mentioned by Andres or the macro trick mentioned by
me.

David

Re: meson files copyright

[Andrew Dunstan]

On 2022-12-19 Mo 15:09, Peter Eisentraut wrote:
> On 19.12.22 19:33, Robert Haas wrote:
>> On Mon, Dec 19, 2022 at 1:03 PM Tom Lane  wrote:
>>> Vik Fearing  writes:
 Perhaps a bit off-topic, but what is the point of the file
 identifiers?
>>>
>>> IMO, it helps to tell things apart when you've got a bunch of editor
>>> windows open on some mighty samey-looking meson.build files.
>>
>> On the other hand, maintaining those identification lines in all of
>> our files has a pretty high distributed cost. I never use them to
>> figure out what file I'm editing because my editor can tell me that.
>> But I do have to keep fixing those lines as I create new files. It's
>> not the most annoying thing ever, but I wouldn't mind a bit if I
>> didn't have to do it any more.
>
> I agree it's not very useful and a bit annoying.


Not sure I agree the cost is high, but yes it's not quite zero either. I
can see a bit more value when it's used with files we have a lot of like
meson.build.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Fixing typo in 002_pg_dump.pl

[Michael Paquier]

On Mon, Dec 19, 2022 at 09:08:54PM +0100, Peter Eisentraut wrote:
> fixed

Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: Error-safe user functions

[Tom Lane]

Robert Haas  writes:
> On Mon, Dec 19, 2022 at 11:44 AM Tom Lane  wrote:
>> ... I guess you didn't read my remarks upthread about regtypein.
>> I do not want to try to make gram.y+scan.l non-error-throwing.

> Huh, for some reason I'm not seeing an email about that. Do you have a link?

In [1] I wrote

>>> I'm a little concerned about the cost-benefit of fixing the reg* types.
>>> The ones that accept type names actually use the core grammar to parse
>>> those.  Now, we probably could fix the grammar to be non-throwing, but
>>> it'd be very invasive and I'm not sure about the performance impact.
>>> It might be best to content ourselves with soft reporting of lookup
>>> failures, as opposed to syntax problems.

regards, tom lane

[1] https://www.postgresql.org/message-id/1863335.1670783397%40sss.pgh.pa.us

Re: Use get_call_result_type() more widely

[Tom Lane]

Robert Haas  writes:
> On Mon, Dec 19, 2022 at 2:07 PM Alvaro Herrera  
> wrote:
>> On the other hand, the measurements have shown that going through the
>> function is significantly slower.  So I kinda like the judgement call
>> that Michael and Bharath have made: change to use the function when
>> performance is not an issue, and keep the verbose coding otherwise.

> Seems fairly arbitrary to me.

Agreed ... but the decisions embodied in the code-as-it-stands are
even more arbitrary, being no doubt mostly based on "which function
did you copy to start from" not on any thought about performance.

Now that somebody's made an effort to identify which places are
potentially performance-critical, I don't see why we wouldn't use
the fruits of their labor.  Yes, somebody else might draw the line
differently, but drawing a line at all seems like a step forward
to me.

regards, tom lane

Re: (non) translatable string splicing

[Robert Haas]

On Fri, Dec 16, 2022 at 8:25 AM Justin Pryzby  wrote:
> Due to incomplete translation, that allows some pretty fancy output,
> like:
> | You must identify the directory where the residen los binarios del clúster 
> antiguo.

I can't see how that could be mejor. :-)

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

Re: Inconsistency in reporting checkpointer stats

[Robert Haas]

On Wed, Dec 14, 2022 at 2:32 AM Nitin Jadhav
 wrote:
> In order to fix this, the
> PendingCheckpointerStats.buf_written_checkpoints should be incremented
> in SlruInternalWritePage() similar to
> CheckpointStats.ckpt_bufs_written. I have attached the patch for the
> same. Please share your thoughts.

Presumably we could make this consistent either by counting SLRU
writes in both places, or by counting them in neither place. This
proposal would count them in both places. But why is that the right
thing to do?

I'm somewhat inclined to think that we should use "buffers" to mean
regular data buffers, and if SLRU buffers also need to be counted, we
ought to make that a separate counter. Or just leave it out
altogether.

This is arguable, though, for sure

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

Re: Add sub-transaction overflow status in pg_stat_activity

[Robert Haas]

On Mon, Dec 19, 2022 at 3:48 PM Ted Yu  wrote:
> It seems the comment for `backend_subxact_overflowed` missed a word.
>
> Please see the patch.

Committed this fix, thanks.

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

Re: Add sub-transaction overflow status in pg_stat_activity

[Ted Yu]

On Mon, Dec 19, 2022 at 11:57 AM Robert Haas  wrote:

> On Tue, Dec 13, 2022 at 2:29 AM Julien Rouhaud  wrote:
> > > > Makes sense.
> > >
> > > +1
> >
> > +1
>
> Committed with a bit more word-smithing on the documentation.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>
> Hi,
It seems the comment for `backend_subxact_overflowed` missed a word.

Please see the patch.

Thanks


subtxn-number-comment.patch
Description: Binary data

Re: appendBinaryStringInfo stuff

[Peter Eisentraut]

On 19.12.22 09:12, Andres Freund wrote:

There are a bunch of places in the json code that use
appendBinaryStringInfo() where appendStringInfoString() could be used, e.g.,

 appendBinaryStringInfo(buf, ".size()", 7);

Is there a reason for this?  Are we that stretched for performance?

strlen() isn't that cheap, so it doesn't generally seem unreasonable. I
don't think we should add the strlen overhead in places that can
conceivably be a bottleneck - and some of the jsonb code clearly can be
that.


AFAICT, the code in question is for the text output of the jsonpath 
type, which is used ... for barely anything?

Re: meson files copyright

[Peter Eisentraut]

On 19.12.22 19:33, Robert Haas wrote:

On Mon, Dec 19, 2022 at 1:03 PM Tom Lane  wrote:

Vik Fearing  writes:

Perhaps a bit off-topic, but what is the point of the file identifiers?


IMO, it helps to tell things apart when you've got a bunch of editor
windows open on some mighty samey-looking meson.build files.


On the other hand, maintaining those identification lines in all of
our files has a pretty high distributed cost. I never use them to
figure out what file I'm editing because my editor can tell me that.
But I do have to keep fixing those lines as I create new files. It's
not the most annoying thing ever, but I wouldn't mind a bit if I
didn't have to do it any more.


I agree it's not very useful and a bit annoying.

Re: Fixing typo in 002_pg_dump.pl

[Peter Eisentraut]

On 19.12.22 19:53, Ted Yu wrote:
I was going over 002_pg_dump.pl  and saw a typo 
in pgdump_runs.


Please see the patch.


fixed

Re: Refactor SCRAM code to dynamically handle hash type and key length

[Jonathan S. Katz]

On 12/16/22 10:08 PM, Michael Paquier wrote:

On Thu, Dec 15, 2022 at 04:59:52AM +0900, Michael Paquier wrote:

However, that's only half of the picture.  The key length and the hash
type (or just the hash type to know what's the digest/key length to
use but that's more invasive) still need to be sent across the
internal routines of SCRAM and attached to the state data of the
frontend and the backend or we won't be able to do the hash and HMAC
computations dependent on that.


Attached is a patch to do exactly that, and as a result v2 is half the
size of v1:
- SCRAM_KEY_LEN is now named SCRAM_MAX_KEY_LEN, adding a note that
this should be kept in sync as the maximum digest size of the
supported hash methods.  This is used as the method to size all the
internal buffers of the SCRAM routines.
- SCRAM_SHA_256_KEY_LEN is used to track the key length for
SCRAM-SHA-256, the one initialized with the state data.
- No changes in the internal, the buffers are just resized based on
the max defined.


Thanks! I looked through this and ran tests. I like the approach overall 
and I think this sets us up pretty well for expanding our SCRAM support.


Only a couple of minor comments:

- I noticed a couple of these in "scram_build_secret" and "scram_mock_salt":

  Assert(hash_type == PG_SHA256);

Presumably there to ensure 1/ We're setting a hash_type and 2/ as 
possibly a reminder to update the assertions if/when we support more 
digests.


With the assertion in "scram_build_secret", that value is set from the 
"PG_SHA256" constant anyway, so I don't know if it actually gives us 
anything other than a reminder? With "scram_mock"salt" the value 
ultimately comes from state (which is currently set from the constant), 
so perhaps there is a guard there.


At a minimum, I'd suggest a comment around it, especially if it's set up 
to be removed at a future date.


- I do like the "SCRAM_MAX_KEY_LEN" change, and I see we're now passing 
"key_length" around to ensure we're only using the desired number of 
bytes. I am a little queasy that once we expand "SCRAM_MAX_KEY_LEN" we 
run the risk of having the smaller hashes accidentally use the extra 
bytes in their calculations. However, I think that's more a fear than 
not, an we can mitigate the risk with testing.



I'd like to move on with that in the next couple of days (still need
to study more the other areas of the code to see what else could be
made more pluggable), so let me know if there are any objections..


No objections. I think this decreases the lift to supporting more 
variations of SCRAM.


Once committed, I'll rebase the server-side SCRAM functions patch with 
this. I may want to rethink the interface for that to allow the digest 
to be "selectable" (vs. from the function) but I'll discuss on that 
thread[1].


Thanks,

Jonathan

[1] 
https://www.postgresql.org/message-id/fce7228e-d0d6-64a1-3dcb-bba85c2fa...@postgresql.org


OpenPGP_signature
Description: OpenPGP digital signature

Re: Add sub-transaction overflow status in pg_stat_activity

[Robert Haas]

On Tue, Dec 13, 2022 at 2:29 AM Julien Rouhaud  wrote:
> > > Makes sense.
> >
> > +1
>
> +1

Committed with a bit more word-smithing on the documentation.

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

Re: Use get_call_result_type() more widely

[Robert Haas]

On Mon, Dec 19, 2022 at 2:07 PM Alvaro Herrera  wrote:
> On the other hand, the measurements have shown that going through the
> function is significantly slower.  So I kinda like the judgement call
> that Michael and Bharath have made: change to use the function when
> performance is not an issue, and keep the verbose coding otherwise.

Seems fairly arbitrary to me. The ones used for monitoring queries
aren't likely to be run often enough that it matters, but in theory
it's possible that they could be. Many of the ones supposedly not used
for monitoring queries could reasonably be so used, too. You can get
any answer you want by making arbitrary assumptions about which ones
are likely to be used frequently and how frequently they're likely to
be used, and I think different people evaluating the list
independently of each other and with no knowledge of each others work
would likely reach substantially different conclusions, ranging all
the way from "do them all this way" to "do them all the other way" and
various positions in the middle.

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

Re: Use get_call_result_type() more widely

[Alvaro Herrera]

On 2022-Dec-19, Robert Haas wrote:

> Here's a modest proposal: let's do nothing about this. There's no
> evidence of a real problem here, so we're going to be trying to judge
> the performance benefits against the code size savings without any
> real data indicating that either one is an issue. I bet we could
> convert all of these to one style or the other and it would make very
> little real world difference, but deciding which ones to change and in
> which direction will take up time and energy that could otherwise be
> spent on more worthwhile projects, and could possibly complicate
> back-patching, too.
> 
> Basically, I think this is nit-picking. Let's just accept that both
> styles have some advantages and leave it up to patch authors to pick
> one that they prefer.

The code savings are substantial actually, so I think bloating things
for cases where performance is not an issue is not good.  Some other
developer is sure to cargo-cult that stuff in the future, and that's not
great.

On the other hand, the measurements have shown that going through the
function is significantly slower.  So I kinda like the judgement call
that Michael and Bharath have made: change to use the function when
performance is not an issue, and keep the verbose coding otherwise.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Fixing typo in 002_pg_dump.pl

[Ted Yu]

Hi,
I was going over 002_pg_dump.pl and saw a typo in pgdump_runs.

Please see the patch.

Thanks


pg-dump-comment.patch
Description: Binary data

Re: Use get_call_result_type() more widely

[Robert Haas]

On Tue, Dec 13, 2022 at 10:43 AM Tom Lane  wrote:
> Saving code is nice, but I'd assume the result is slower, because
> get_call_result_type has to do a pretty substantial amount of work
> to get the data to construct the tupdesc from.  Have you tried to
> quantify how much overhead this'd add?  Which of these functions
> can we safely consider to be non-performance-critical?

Here's a modest proposal: let's do nothing about this. There's no
evidence of a real problem here, so we're going to be trying to judge
the performance benefits against the code size savings without any
real data indicating that either one is an issue. I bet we could
convert all of these to one style or the other and it would make very
little real world difference, but deciding which ones to change and in
which direction will take up time and energy that could otherwise be
spent on more worthwhile projects, and could possibly complicate
back-patching, too.

Basically, I think this is nit-picking. Let's just accept that both
styles have some advantages and leave it up to patch authors to pick
one that they prefer.

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

Re: meson files copyright

[Robert Haas]

On Mon, Dec 19, 2022 at 1:03 PM Tom Lane  wrote:
> Vik Fearing  writes:
> > Perhaps a bit off-topic, but what is the point of the file identifiers?
>
> IMO, it helps to tell things apart when you've got a bunch of editor
> windows open on some mighty samey-looking meson.build files.

On the other hand, maintaining those identification lines in all of
our files has a pretty high distributed cost. I never use them to
figure out what file I'm editing because my editor can tell me that.
But I do have to keep fixing those lines as I create new files. It's
not the most annoying thing ever, but I wouldn't mind a bit if I
didn't have to do it any more.

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

Re: Error-safe user functions

[Robert Haas]

On Mon, Dec 19, 2022 at 11:44 AM Tom Lane  wrote:
> > It also doesn't seem too bad from an implementation point of view to
> > try to cover all the caes.
>
> ... I guess you didn't read my remarks upthread about regtypein.
> I do not want to try to make gram.y+scan.l non-error-throwing.

Huh, for some reason I'm not seeing an email about that. Do you have a link?

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

Re: meson files copyright

[Tom Lane]

Vik Fearing  writes:
> Perhaps a bit off-topic, but what is the point of the file identifiers?

IMO, it helps to tell things apart when you've got a bunch of editor
windows open on some mighty samey-looking meson.build files.

regards, tom lane

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Mon, Dec 19, 2022 at 01:06:00PM +0900, Michael Paquier wrote:
> On Sat, Dec 17, 2022 at 05:26:15PM -0600, Justin Pryzby wrote:
> > 001: still refers to "gzip", which is correct for -Fp and -Fd but not
> > for -Fc, for which it's more correct to say "zlib".
> 
> Or should we begin by changing all these existing "not built with zlib 
> support" error strings to the more generic "this build does not
> support compression with %s" to reduce the number of messages to
> translate?  That would bring consistency with the other tools dealing
> with compression.

That's fine, but it doesn't touch on the issue I'm talking about, which
is that zlib != gzip.

BTW I noticed that that also affects the pg_dump file itself; 002
changes the file format to say "gzip", but that's wrong for -Fc, which
does not use gzip headers, which could be surprising to someone who
specified "gzip".

-- 
Justin

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Mon, Dec 19, 2022 at 05:03:21PM +, gkokola...@pm.me wrote:
> > > 001 still doesn't compile on freebsd, and 002 doesn't compile on
> > > windows. Have you checked test results from cirrusci on your private
> > > github account ?
> 
> There are still known gaps in 0002 and 0003, for example documentation,
> and I have not been focusing too much on those. You are right, it is helpful
> and kind to try to reduce the noise. The attached should have hopefully
> tackled the ci errors.

Yep.  Are you using cirrusci under your github account ?

> > FYI, I have re-added an entry to the CF app to get some automated
> > coverage:
> > https://commitfest.postgresql.org/41/3571/
> 
> Much obliged. Should I change the state to "ready for review" when post a
> new version or should I leave that to the senior personnel?   

It's better to update it to reflect what you think its current status
is.  If you think it's ready for review.

> > > 002 breaks "pg_dump -Fc -Z2" because (I think) AllocateCompressor()
> > > doesn't store the passed-in compression_spec.
> 
> I am afraid I have not been able to reproduce this error. I tried both
> debian and freebsd after I addressed the compilation warnings. Which
> error did you get? Is it still present in the attached?

It's not that there's an error - it's that compression isn't working.

$ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z1 -Fp regression |wc -c
659956
$ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z2 -Fp regression |wc -c
637192

$ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z1 -Fc regression |wc -c
1954890
$ ./tmp_install/usr/local/pgsql/bin/pg_dump -h /tmp -Z2 -Fc regression |wc -c
1954890

-- 
Justin

Re: meson files copyright

[Vik Fearing]

On 12/19/22 16:20, Tom Lane wrote:

Andrew Dunstan  writes:

I notice that none of the meson files contain copyright notices. Shall I
add them?


+1.  Their comment density is pretty awful too --- maybe I'm just
not used to meson, but they seem just about completely undocumented.
And there's certainly been no effort to transfer the accumulated wisdom
of the makefile comments (where it's still relevant, of course).

Don't see any simple fix for that, but copyright notices would
be a good idea, and so would file identifiers according to our
longstanding practice.


Perhaps a bit off-topic, but what is the point of the file identifiers?
--
Vik Fearing

Re: GROUP BY ALL

[Vik Fearing]

On 12/19/22 05:19, Andrey Borodin wrote:

Hi hackers!

I saw a thread in a social network[0] about GROUP BY ALL. The idea seems useful.
I always was writing something like
 select datname, usename, count(*) from pg_stat_activity group by 1,2;
and then rewriting to
 select datname, usename, query, count(*) from pg_stat_activity group by 
1,2;
and then "aaa, add a number at the end".

With the proposed feature I can write just
 select datname, usename, count(*) from pg_stat_activity group by all;



We already have GROUP BY ALL, but it doesn't do this.



PFA very dummy implementation just for a discussion. I think we can
add all non-aggregating targets.

What do you think?



I think this is a pretty terrible idea.  If we want that kind of 
behavior, we should just allow the GROUP BY to be omitted since without 
grouping sets, it is kind of redundant anyway.


I don't know what my opinion is on that.
--
Vik Fearing

Re: Error-safe user functions

[Tom Lane]

Robert Haas  writes:
> On Fri, Dec 16, 2022 at 1:31 PM Tom Lane  wrote:
>> The reg* functions probably need a unified plan as to how far
>> down we want to push non-error behavior.  The rest of these
>> I think just require turning the crank along the same lines
>> as in functions already dealt with.

> I would be in favor of an aggressive approach.

I agree that anything based on implementation concerns is going
to look pretty unprincipled to end users.  However ...

> It also doesn't seem too bad from an implementation point of view to
> try to cover all the caes.

... I guess you didn't read my remarks upthread about regtypein.
I do not want to try to make gram.y+scan.l non-error-throwing.

regards, tom lane

Re: Common function for percent placeholder replacement

[Robert Haas]

On Mon, Dec 19, 2022 at 3:13 AM Peter Eisentraut
 wrote:
> I agree.  Here is an updated patch with the error checking included.

Nice, but I think something in the error report needs to indicate what
caused the problem exactly. As coded, I think the user would have to
guess which GUC caused the problem. For basebackup_to_shell that might
not be too hard since you would have to try to initiate a backup to a
shell target to trigger the error, but for something that happens at
server start, you don't want to have to go search all of
postgresql.conf for possible causes.

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

Re: Error-safe user functions

[Robert Haas]

On Fri, Dec 16, 2022 at 1:31 PM Tom Lane  wrote:
> The reg* functions probably need a unified plan as to how far
> down we want to push non-error behavior.  The rest of these
> I think just require turning the crank along the same lines
> as in functions already dealt with.

I would be in favor of an aggressive approach. For example, let's look
at regclassin(). It calls oidin(), stringToQualifiedNameList(),
makeRangeVarFromNameList(), and RangeVarGetRelidExtended(). Basically,
oidin() could fail if the input, known to be all digits, is out of
range; stringToQualifiedNameList() could fail due to mismatched
delimiters or improperly-separated names; makeRangeVarFromNameList()
doesn't want to have more than three name components
(db.schema.relation); and RangeVarGetRelidExtended() doesn't like
cross-database references or non-existent relations.

Now, one option here would be to distinguish between something that
could be valid in some database but isn't in this one, like a
non-existent relation name, and one that couldn't ever work anywhere,
like a relation name with four parts or bad quoting. You could decide
that the former kind of error will be reported softly but the latter
is hard error. But I think that is presuming that we know how users
will want to use this functionality, and I don't think we do. I also
think that it will be confusing to users. Finally, I think it's
different from what we do for other data types. You could equally well
argue that, for int4in, we ought to treat '99' and 'potato'
differently, one a hard error and the other soft. I think it's hard to
puzzle out a decision that makes any sense there, and I don't think
this case is much different. I don't think it's too hard to mentally
separate errors about the validity of the input from, say, out of
memory errors -- but one distinguishing between one kind of input
validity check and another seems like a muddle.

It also doesn't seem too bad from an implementation point of view to
try to cover all the caes. The stickiest case looks to be
RangeVarGetRelidExtended() and we might need to give a bit of thought
to how to handle that one. The others don't seem like a big issue, and
oidin() is already done.

> While it'd be good to get all of these done before v16 feature
> freeze, I can't see that any of them represent blockers for
> building features based on soft input error handling.

+1.

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

Add a hook to allow modification of the ldapbindpasswd

[Andrew Dunstan]

This patch, mostly the work of John Naylor, provides a hook whereby a
module can modify the ldapbindpasswd before it is handed to the ldap
server. This is similar in concept to the ssl_passphrase_callback
feature, and allows the user not to have to put the cleartext password
in the pg_hba.conf file. A trivial test is added which provides an
example of such a module.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 65af40c5b3c05dcfcb55675dec066fe779382105 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Mon, 19 Dec 2022 11:19:24 -0500
Subject: [PATCH] Add a password handling hook for ldapbindpasswd

This hook allows for interception of the ldapbindpasswd in the
pg_hba.conf file before it is passed on to the ldap server. The hook
function receives a copy of the password as specified in the config file
and hands back a pointer to a password string. the default handler
simply hands back its input. A test is supplied which implements a
module that makes a trivial (rot13) modifiction of the input. The
benefit here is that the clear text password no longer needs to be
stored in the config file. This is similar in concept to the
ssl_passphrase_callback feature.

John Naylor, with small modifications by Andrew Dunstan.
---
 src/backend/libpq/auth.c  |  12 +-
 src/include/libpq/auth.h  |   6 +
 src/test/modules/Makefile |  11 +
 src/test/modules/ldap_password_func/Makefile  |  25 ++
 .../modules/ldap_password_func/authdata.ldif  |  34 +++
 .../ldap_password_func/ldap_password_func.c   |  64 +
 .../modules/ldap_password_func/meson.build|  35 +++
 .../t/001_mutated_bindpasswd.pl   | 220 ++
 src/test/modules/meson.build  |   1 +
 9 files changed, 407 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/ldap_password_func/Makefile
 create mode 100644 src/test/modules/ldap_password_func/authdata.ldif
 create mode 100644 src/test/modules/ldap_password_func/ldap_password_func.c
 create mode 100644 src/test/modules/ldap_password_func/meson.build
 create mode 100644 src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index e2f723d188..1ba2fa92ab 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -144,6 +144,10 @@ static int	CheckLDAPAuth(Port *port);
 #define LDAP_OPT_DIAGNOSTIC_MESSAGE LDAP_OPT_ERROR_STRING
 #endif
 
+/* Default LDAP password mutator hook, can be overridden by a shared library */
+static char*  dummy_ldap_password_mutator(char* input);
+auth_password_hook_typ ldap_password_hook = dummy_ldap_password_mutator;
+
 #endif			/* USE_LDAP */
 
 /*
@@ -2370,6 +2374,12 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
 #define LDAPS_PORT 636
 #endif
 
+static char*
+dummy_ldap_password_mutator(char * input)
+{
+	return input;
+}
+
 /*
  * Return a newly allocated C string copied from "pattern" with all
  * occurrences of the placeholder "$username" replaced with "user_name".
@@ -2498,7 +2508,7 @@ CheckLDAPAuth(Port *port)
 		 */
 		r = ldap_simple_bind_s(ldap,
 			   port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
-			   port->hba->ldapbindpasswd ? port->hba->ldapbindpasswd : "");
+			   port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
 		if (r != LDAP_SUCCESS)
 		{
 			ereport(LOG,
diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h
index d3c189efe3..c29bd17516 100644
--- a/src/include/libpq/auth.h
+++ b/src/include/libpq/auth.h
@@ -28,4 +28,10 @@ extern void sendAuthRequest(Port *port, AuthRequest areq, const char *extradata,
 typedef void (*ClientAuthentication_hook_type) (Port *, int);
 extern PGDLLIMPORT ClientAuthentication_hook_type ClientAuthentication_hook;
 
+/* hook type for password manglers */
+typedef char* (* auth_password_hook_typ)(char* input);
+
+/* Default LDAP password mutator hook, can be overridden by a shared library */
+extern PGDLLIMPORT auth_password_hook_typ ldap_password_hook;
+
 #endif			/* AUTH_H */
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index c629cbe383..79e3033ec2 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -42,5 +42,16 @@ else
 ALWAYS_SUBDIRS += ssl_passphrase_callback
 endif
 
+# Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
+ifeq ($(with_ldap),yes)
+ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+SUBDIRS += ldap_password_func
+else
+ALWAYS_SUBDIRS += ldap_password_func
+endif
+else
+ALWAYS_SUBDIRS += ldap_password_func
+endif
+
 $(recurse)
 $(recurse_always)
diff --git a/src/test/modules/ldap_password_func/Makefile b/src/test/modules/ldap_password_func/Makefile
new file mode 100644
index 00..3324e04248
--- /dev/null
+++ b/src/test/modules/ldap_password_func/Makefile
@@ -0,0 +1,25 @@
+# Copyright (c) 2022, PostgreSQL 

Add a test to ldapbindpasswd

[Andrew Dunstan]

There is currently no test for the use of ldapbindpasswd in the
pg_hba.conf file. This patch, mostly the work of John Naylor, remedies that.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 78ca6b405601ce0d884406b94fa356e38e19d2e0 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Mon, 19 Dec 2022 06:38:11 -0500
Subject: [PATCH] Add a test for ldapbindpasswd

The existing LDAP tests don't cover the use of ldapbindpasswd in
pg_hba.conf, so remedy that.

Author: John Naylor
---
 src/test/ldap/meson.build |   1 +
 src/test/ldap/t/002_bindpasswd.pl | 198 ++
 2 files changed, 199 insertions(+)
 create mode 100644 src/test/ldap/t/002_bindpasswd.pl

diff --git a/src/test/ldap/meson.build b/src/test/ldap/meson.build
index 8577385ee5..4c8df92a26 100644
--- a/src/test/ldap/meson.build
+++ b/src/test/ldap/meson.build
@@ -5,6 +5,7 @@ tests += {
   'tap': {
 'tests': [
   't/001_auth.pl',
+	  't/002_bindpasswd.pl',
 ],
 'env': {
   'with_ldap': ldap.found() ? 'yes' : 'no',
diff --git a/src/test/ldap/t/002_bindpasswd.pl b/src/test/ldap/t/002_bindpasswd.pl
new file mode 100644
index 00..0ed3a630af
--- /dev/null
+++ b/src/test/ldap/t/002_bindpasswd.pl
@@ -0,0 +1,198 @@
+
+# Copyright (c) 2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+use File::Copy;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+
+my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
+
+$ldap_bin_dir = undef;# usually in PATH
+
+if ($ENV{with_ldap} ne 'yes')
+{
+	plan skip_all => 'LDAP not supported by this build';
+}
+elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+{
+	plan skip_all => 'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
+}
+elsif ($^O eq 'darwin' && -d '/usr/local/opt/openldap')
+{
+	# typical paths for Homebrew
+	$slapd   = '/usr/local/opt/openldap/libexec/slapd';
+	$ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}
+elsif ($^O eq 'darwin' && -d '/opt/local/etc/openldap')
+{
+	# typical paths for MacPorts
+	$slapd   = '/opt/local/libexec/slapd';
+	$ldap_schema_dir = '/opt/local/etc/openldap/schema';
+}
+elsif ($^O eq 'linux')
+{
+	$slapd   = '/usr/sbin/slapd';
+	$ldap_schema_dir = '/etc/ldap/schema' if -d '/etc/ldap/schema';
+	$ldap_schema_dir = '/etc/openldap/schema' if -d '/etc/openldap/schema';
+}
+elsif ($^O eq 'freebsd')
+{
+	$slapd   = '/usr/local/libexec/slapd';
+	$ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}
+elsif ($^O eq 'openbsd')
+{
+	$slapd   = '/usr/local/libexec/slapd';
+	$ldap_schema_dir = '/usr/local/share/examples/openldap/schema';
+}
+else
+{
+	plan skip_all => "ldap tests not supported on $^O or dependencies not installed";
+}
+
+# make your own edits here
+#$slapd = '';
+#$ldap_bin_dir = '';
+#$ldap_schema_dir = '';
+
+$ENV{PATH} = "$ldap_bin_dir:$ENV{PATH}" if $ldap_bin_dir;
+
+my $ldap_datadir  = "${PostgreSQL::Test::Utils::tmp_check}/openldap-data";
+my $slapd_certs   = "${PostgreSQL::Test::Utils::tmp_check}/slapd-certs";
+my $slapd_conf= "${PostgreSQL::Test::Utils::tmp_check}/slapd.conf";
+my $slapd_pidfile = "${PostgreSQL::Test::Utils::tmp_check}/slapd.pid";
+my $slapd_logfile = "${PostgreSQL::Test::Utils::log_path}/slapd.log";
+my $ldap_conf = "${PostgreSQL::Test::Utils::tmp_check}/ldap.conf";
+my $ldap_server   = 'localhost';
+my $ldap_port = PostgreSQL::Test::Cluster::get_free_port();
+my $ldaps_port= PostgreSQL::Test::Cluster::get_free_port();
+my $ldap_url  = "ldap://$ldap_server:$ldap_port;;
+my $ldaps_url = "ldaps://$ldap_server:$ldaps_port";
+my $ldap_basedn   = 'dc=example,dc=net';
+my $ldap_rootdn   = 'cn=Manager,dc=example,dc=net';
+my $ldap_rootpw   = 'secret';
+my $ldap_pwfile   = "${PostgreSQL::Test::Utils::tmp_check}/ldappassword";
+
+note "setting up slapd";
+
+# need to create new config without anonymous auth
+unlink $slapd_conf;
+
+append_to_file(
+	$slapd_conf,
+	qq{include $ldap_schema_dir/core.schema
+include $ldap_schema_dir/cosine.schema
+include $ldap_schema_dir/nis.schema
+include $ldap_schema_dir/inetorgperson.schema
+
+pidfile $slapd_pidfile
+logfile $slapd_logfile
+
+access to *
+by * read
+by users auth
+
+database ldif
+directory $ldap_datadir
+
+TLSCACertificateFile $slapd_certs/ca.crt
+TLSCertificateFile $slapd_certs/server.crt
+TLSCertificateKeyFile $slapd_certs/server.key
+
+suffix "dc=example,dc=net"
+rootdn "$ldap_rootdn"
+rootpw $ldap_rootpw});
+
+copy "../ssl/ssl/server_ca.crt", "$slapd_certs/ca.crt"
+  || die "copying ca.crt: $!";
+copy "../ssl/ssl/server-cn-only.crt", "$slapd_certs/server.crt"
+  || die "copying server.crt: $!";;
+copy "../ssl/ssl/server-cn-only.key", "$slapd_certs/server.key"
+  || die "copying server.key: $!";;
+
+system_or_bail $slapd, '-f', $slapd_conf, '-h', "$ldap_url $ldaps_url";
+
+END
+{
+	kill 'INT', `cat $slapd_pidfile` if -f $slapd_pidfile;
+}
+
+# wait until slapd accepts requests
+my $retries = 

Re: Avoid generating SSL certs for LDAP tests

[Andrew Dunstan]

On 2022-12-19 Mo 10:25, Tom Lane wrote:
> Andrew Dunstan  writes:
>> We don't generate SSL certificates for running the SSL tests, but
>> instead use pregenerated certificates that are part of our source code.
>> This patch applies the same policy to the LDAP tests, and in fact simply
>> reuses certificates from the SSL test suite by copying them. It won't
>> save much but it should save a handful of cycles at run time.
> +1, but should there be a comment somewhere under test/ssl pointing
> out this external use of the certs?


OK, I'll find a place to mention that.


> Also, I bet this needs some adjustment for VPATH builds.  


I have tested it with both a make style vpath build and with meson - it
works fine.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Patch: Global Unique Index

[Nikita Malakhov]

Hi!

Sorry to bother - but is this patch used in IvorySQL?
Here:
https://www.ivorysql.org/docs/Global%20Unique%20Index/create_global_unique_index
According to syntax it definitely looks like this patch.
Thank you!


On Sat, Dec 3, 2022 at 3:05 AM David Zhang  wrote:

> On 2022-11-29 6:16 p.m., Tom Lane wrote:
> > Assuming that you are inserting into index X, and you've checked
> > index Y to find that it has no conflicts, what prevents another
> > backend from inserting a conflict into index Y just after you look?
> > AIUI the idea is to prevent that by continuing to hold an exclusive
> > lock on the whole index Y until you've completed the insertion.
> > Perhaps there's a better way to do that, but it's not what was
> > described.
> Another main change in patch
> `0004-support-global-unique-index-insert-and-update.patch`,
> +search_global:
> +stack = _bt_search(iRel, insertstate.itup_key,
> +   , BT_READ,
> NULL);
> +xwait = _bt_check_unique_gi(iRel, ,
> +hRel, checkUnique,
> _unique,
> + , heapRel);
> +if (unlikely(TransactionIdIsValid(xwait)))
> +{
> ... ...
> +goto search_global;
> +}
>
> Here, I am trying to use `BT_READ` to require a LW_SHARED lock on the
> buffer block if a match found using `itup_key` search key. The
> cross-partition uniqueness checking will wait if the index tuple
> insertion on this buffer block has not done yet, otherwise runs the
> uniqueness check to see if there is an ongoing transaction which may
> insert a conflict value. Once the ongoing insertion is done, it will go
> back and check again (I think it can also handle the case that a
> potential conflict index tuple was later marked as dead in the same
> transaction). Based on this change, my test results are:
>
> 1) a select-only query will not be blocked by the ongoing insertion on
> index X
>
> 2) insertion happening on index Y may wait for the buffer block lock
> when inserting a different value but it does not wait for the
> transaction lock held by insertion on index X.
>
> 3) when an insertion inserting a conflict value on index Y,
>  3.1) it waits for buffer block lock if the lock has been held by
> the insertion on index X.
>  3.2) then, it waits for transaction lock until the insertion on
> index X is done.
>
>
>
>

-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

Re: Support logical replication of DDLs

[Alvaro Herrera]

On 2022-Oct-31, Peter Smith wrote:

> 6. add_policy_clauses
> 
> + else
> + {
> + append_bool_object(policyStmt, "present", false);
> + }
> 
> Something seems strange. Probably I'm wrong but just by code
> inspection it looks like there is potential for there to be multiple
> param {present:false} JSON objects:
> 
> {"present" :false},
> {"present" :false},
> {"present" :false},
> 
> Shouldn't those all be array elements or something? IIUC apart from
> just DDL, the JSON idea was going to (in future) allow potential
> machine manipulation of the values prior to the replication, but
> having all these ambiguous-looking objects does not seem to lend
> itself to that idea readily. How to know what are each of those params
> representing?

Do you mean that a single JSON object has multiple member with
"present":"false"?  That sounds like something we should never produce,
and post-processing to remove them does not sound good either.  Is that
really what is happening, or do I misunderstand?

Obviously, if you have an object with several sub-objects, each of the
sub-objects can have its own "present:false" label.  The idea is that
the clause that each subobject represents may not be in the command as
written by the user; but perhaps a post-processor of the JSON blob wants
to change things so that the clause does appear in the final output.
And this should be doable for each individual optional clause in each
command, which means that, yeah, there should be multiple
"present:false" pairs in a single JSON blob, in different paths.

(For example, if the user writes "CREATE SEQUENCE foobar", we would get
a tree that has {fmt: "CACHE %{value}", present: false, value: 32}, so
if you just convert that to text DDL without further ado you would get
the original command verbatim; but you can poke the "present" to true so
you would get "CREATE SEQUENCE foobar CACHE 32".)


Also, I think I came up with the idea of having "present:boolean" a bit
late in the development of this code, so it's quite possible that there
are commands that are inconsistent in their support of this pattern.
That makes it especially important to review the representation of each
command carefully.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

Re: Avoid generating SSL certs for LDAP tests

[Tom Lane]

Andrew Dunstan  writes:
> We don't generate SSL certificates for running the SSL tests, but
> instead use pregenerated certificates that are part of our source code.
> This patch applies the same policy to the LDAP tests, and in fact simply
> reuses certificates from the SSL test suite by copying them. It won't
> save much but it should save a handful of cycles at run time.

+1, but should there be a comment somewhere under test/ssl pointing
out this external use of the certs?

Also, I bet this needs some adjustment for VPATH builds.

regards, tom lane

Re: meson files copyright

[Tom Lane]

Andrew Dunstan  writes:
> I notice that none of the meson files contain copyright notices. Shall I
> add them?

+1.  Their comment density is pretty awful too --- maybe I'm just
not used to meson, but they seem just about completely undocumented.
And there's certainly been no effort to transfer the accumulated wisdom
of the makefile comments (where it's still relevant, of course).

Don't see any simple fix for that, but copyright notices would
be a good idea, and so would file identifiers according to our
longstanding practice.

regards, tom lane

Re: appendBinaryStringInfo stuff

[Tom Lane]

David Rowley  writes:
> I'm currently not sure why the macro trick increases the binary at
> all. I understand why the inline function does.

In the places where it changes the code at all, you're replacing

appendStringInfoString(buf, s);

with

appendBinaryStringInfo(buf, s, n);

Even if n is a constant, the latter surely requires more instructions
per call site.

Whether this is a win seems to depend on how many of these are
performance-critical.

regards, tom lane

Avoid generating SSL certs for LDAP tests

[Andrew Dunstan]

We don't generate SSL certificates for running the SSL tests, but
instead use pregenerated certificates that are part of our source code.
This patch applies the same policy to the LDAP tests, and in fact simply
reuses certificates from the SSL test suite by copying them. It won't
save much but it should save a handful of cycles at run time.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 5a58ef37a20cd229a74ffa4b9b5d52a47a38f020 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Mon, 19 Dec 2022 05:58:08 -0500
Subject: [PATCH] Use existing SSL certs in LDAP tests instead of generating
 them

The SSL test suite has a bunch of pre-existing certificates, so it's
better simply to use what we already have than generate new certificates
each time the LDAP tests are run.
---
 src/test/ldap/Makefile  |  1 -
 src/test/ldap/meson.build   |  1 -
 src/test/ldap/t/001_auth.pl | 19 ---
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/src/test/ldap/Makefile b/src/test/ldap/Makefile
index b1e4a7be67..e5fa3d8610 100644
--- a/src/test/ldap/Makefile
+++ b/src/test/ldap/Makefile
@@ -14,7 +14,6 @@ top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global
 
 export with_ldap
-export OPENSSL
 
 check:
 	$(prove_check)
diff --git a/src/test/ldap/meson.build b/src/test/ldap/meson.build
index 020f6e7f08..8577385ee5 100644
--- a/src/test/ldap/meson.build
+++ b/src/test/ldap/meson.build
@@ -8,7 +8,6 @@ tests += {
 ],
 'env': {
   'with_ldap': ldap.found() ? 'yes' : 'no',
-  'OPENSSL': openssl.path(),
 },
   },
 }
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index fd90832b75..0ea274c383 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -3,6 +3,7 @@
 
 use strict;
 use warnings;
+use File::Copy;
 use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
 use Test::More;
@@ -113,17 +114,13 @@ append_to_file(
 mkdir $ldap_datadir or die;
 mkdir $slapd_certs  or die;
 
-my $openssl = $ENV{OPENSSL};
-
-system_or_bail $openssl, "req", "-new", "-nodes", "-keyout",
-  "$slapd_certs/ca.key", "-x509", "-out", "$slapd_certs/ca.crt", "-subj",
-  "/CN=CA";
-system_or_bail $openssl, "req", "-new", "-nodes", "-keyout",
-  "$slapd_certs/server.key", "-out", "$slapd_certs/server.csr", "-subj",
-  "/CN=server";
-system_or_bail $openssl, "x509", "-req", "-in", "$slapd_certs/server.csr",
-  "-CA", "$slapd_certs/ca.crt", "-CAkey", "$slapd_certs/ca.key",
-  "-CAcreateserial", "-out", "$slapd_certs/server.crt";
+# use existing certs from nearby SSL test suite
+copy "../ssl/ssl/server_ca.crt", "$slapd_certs/ca.crt"
+  || die "copying ca.crt: $!";
+copy "../ssl/ssl/server-cn-only.crt", "$slapd_certs/server.crt"
+  || die "copying server.crt: $!";;
+copy "../ssl/ssl/server-cn-only.key", "$slapd_certs/server.key"
+  || die "copying server.key: $!";;
 
 system_or_bail $slapd, '-f', $slapd_conf, '-h', "$ldap_url $ldaps_url";
 
-- 
2.34.1

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Maxim Orlov]

Hi!

As a result of discussion in the thread [0], Robert Haas proposed to focus
on making SLRU 64 bit, as a first step towards 64 bit XIDs.
Here is the patch set.

In overall, code of this patch set is based on the existing code from [0]
and may be simplified, due to the fact, that SLRU_PAGES_PER_SEGMENT is not
meant to be changed now.
But I decided to leave it that way. At least for now.

As always, reviews and opinions are very welcome.

Should we change status for this thread to "need review"?

[0]
https://www.postgresql.org/message-id/flat/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com#03a4ab82569bb7b112db4a2f352d96b9

-- 
Best regards,
Maxim Orlov.


v51-0003-Make-pg_upgrade-from-32-bit-to-64-bit-SLRU.patch
Description: Binary data


v51-0001-Use-internal-64-bit-numbering-of-SLRU-pages.patch
Description: Binary data


v51-0002-Use-64-bit-pages-representation-in-SLRU-callers.patch
Description: Binary data

Re: Use get_call_result_type() more widely

[Bharath Rupireddy]

On Thu, Dec 15, 2022 at 11:41 AM Michael Paquier  wrote:
>
> On Wed, Dec 14, 2022 at 11:14:59AM +0530, Bharath Rupireddy wrote:
> > AFAICS, most of these functions have no direct source code callers,
> > they're user-facing functions and not in a hot code path. I measured
> > the test times of these functions and I don't see much difference [1].
>
> Thanks for the summary.  It looks like your tests involve single
> runs.  What is the difference in run-time when invoking this
> repeatedly with a large generate_series() for example when few or no
> tuples are returned?  Do you see a difference in perf profile?  Some
> of the functions could have their time mostly eaten while looking at
> the syscache on repeated calls, but you could see the actual work this
> involves with a dummy function that returns a large number of
> attributes on a single record in the worst case possible?

Thanks. Yes, using get_call_result_type() for a function that gets
called repeatedly does have some cost as the comment around
get_call_result_type() says - I found in my testing that
get_call_result_type() does seem to cost 45% increase in execution
times over quick iterations of a function returning a single row with
36 columns.

> Separating things into two buckets..
>
> > [1]
> > pg_old_snapshot_time_mapping() - an extension function with no
> > internal source code callers, no test coverage.
> > pg_visibility_map_summary() - an extension function with no internal
> > source code callers, test coverage exists, test times on HEAD:25 ms
> > PATCHED:25 ms
> > pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no
> > internal source code callers, test coverage exists, test times on
> > HEAD:10 ms PATCHED:10 ms> pg_get_multixact_members() - no internal source 
> > code callers, no test coverage.
> > pg_control_recovery() and pg_control_init() - test coverage exists,
> > test times on HEAD:42 ms PATCHED:44 ms
> > pg_identify_object() - no internal source code callers, test coverage
> > exists, test times on HEAD:17 ms PATCHED:18 ms
> > pg_identify_object_as_address() - no internal source code callers,
> > test coverage exists, test times on HEAD:66 ms PATCHED:60 ms
> > pg_get_object_address() - no internal source code callers, test
> > coverage exists, test times on HEAD:66 ms PATCHED:60 ms
> > pg_sequence_parameters() - no internal source code callers, test
> > coverage exists, test times on HEAD:96 ms PATCHED:98 ms
> > ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and
> > ts_parse_byname() - internal source code callers exists, test coverage
> > exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs
> > PATCHED:186 ms, pg_dump 10 wallclock secs
> > pg_get_keywords() - no internal source code callers, test coverage
> > exists, test times on HEAD:129 ms PATCHED:130 ms
> > pg_get_catalog_foreign_keys() - no internal source code callers, test
> > coverage exists, test times on HEAD:114 ms PATCHED:111 ms
> > tsvector_unnest() - no internal source code callers, test coverage
> > exists, test times on HEAD:26 ms PATCHED:26 ms
> > ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms
> > PATCHED:186 ms
> > pg_partition_tree() - no internal source code callers, test coverage
> > exists, test times on HEAD:30 ms PATCHED:32 ms
> > pg_timezone_abbrevs() - no internal source code callers, test coverage
> > exists, test times on HEAD:40 ms PATCHED:36 ms
>
> These ones don't worry me much, TBH.
>
> > pg_stat_get_wal(), pg_stat_get_archiver() and
> > pg_stat_get_replication_slot() - no internal source code callers, test
> > coverage exists, test times on HEAD:479 ms PATCHED:483 ms
> > pg_prepared_xact() - no internal source code callers, test coverage
> > exists, test times on HEAD:50 ms, subscription 108 wallclock secs,
> > recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock
> > secs, recovery 112 wallclock secs
> > show_all_settings(), pg_control_system(), pg_control_checkpoint(),
> > test_predtest() - no internal source code callers, test coverage
> > exists, test times on HEAD:18 ms PATCHED:18 ms
> > pg_walfile_name_offset() - no internal source code callers, no test 
> > coverage.
> > aclexplode() - internal callers exists information_schema.sql,
> > indirect test coverage exists.
> > pg_stat_file() - no internal source code callers, test coverage
> > exists, test times on HEAD:42 ms PATCHED:46 ms
> > pg_get_publication_tables() - internal source code callers exist, test
> > coverage exists, test times on HEAD:159 ms, subscription 108 wallclock
> > secs PATCHED:167 ms, subscription 110 wallclock secs
> > pg_lock_status() - no internal source code callers, test coverage
> > exists, test times on HEAD:16 ms PATCHED:22 ms
> > pg_stat_get_subscription_stats() - no internal source code callers,
> > test coverage exists, test times on HEAD:subscription 108 wallclock
> > secs PATCHED:subscription 110 wallclock secs
>
> These ones could be involved in monitoring queries run on a 

meson files copyright

[Andrew Dunstan]

I notice that none of the meson files contain copyright notices. Shall I
add them?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: GROUP BY ALL

[Isaac Morland]

On Sun, 18 Dec 2022 at 23:30, Tom Lane  wrote:

> Andrey Borodin  writes:
> > I saw a thread in a social network[0] about GROUP BY ALL. The idea seems
> useful.
>
> Isn't that just a nonstandard spelling of SELECT DISTINCT?
>

In a pure relational system, yes; but since Postgres allows duplicate rows,
both in actual table data and in intermediate and final result sets, no.
Although I'm pretty sure no aggregates other than count() are useful - any
other aggregate would always just combine count() copies of the duplicated
value in some way.

What would happen if there are aggregate functions in the tlist?
> I'm not especially on board with "ALL" meaning "ALL (oh, but not
> aggregates)".
>

The requested behaviour can be accomplished by an invocation something like:

select (t).*, count(*) from (select (…field1, field2, …) as t from
…tables…) s group by t;

So we collect all the required fields as a tuple, group by the tuple, and
then unpack it into separate columns in the outer query.

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

[Amit Kapila]

On Sat, Dec 17, 2022 at 7:34 PM houzj.f...@fujitsu.com
 wrote:
>
> Agreed. I have addressed all the comments and did some cosmetic changes.
> Attach the new version patch set.
>

Few comments:

1.
+ if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
+ {
+ pa_lock_stream(MyParallelShared->xid, AccessShareLock);
+ pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
+ }
+
+ /*
+ * We cannot read the file immediately after the leader has serialized all
+ * changes to the file because there may still be messages in the memory
+ * queue. We will apply all spooled messages the next time we call this
+ * function, which should ensure that there are no messages left in the
+ * memory queue.
+ */
+ else if (fileset_state == FS_SERIALIZE_DONE)
+ {

Once we have waited in the FS_SERIALIZE_IN_PROGRESS, the file state
can be FS_SERIALIZE_DONE immediately after that. So, won't it be
better to have a separate if block for FS_SERIALIZE_DONE state? If you
agree to do so then we can probably remove the comment: "* XXX It is
possible that immediately after we have waited for a lock in ...".

2.
+void
+pa_decr_and_wait_stream_block(void)
+{
+ Assert(am_parallel_apply_worker());
+
+ if (pg_atomic_sub_fetch_u32(>pending_stream_count, 1) == 0)

I think here the count can go negative when we are in serialize mode
because we don't increase it for serialize mode. I can't see any
problem due to that but OTOH, this doesn't seem to be intended because
in the future if we decide to implement the functionality of switching
back to non-serialize mode, this could be a problem. Also, I guess we
don't even need to try locking/unlocking the stream lock in that case.
One idea to avoid this is to check if the pending count is zero then
if file_set in not available raise an error (elog ERROR), otherwise,
simply return from here.

3. In apply_handle_stream_stop(), we are setting backendstate as idle
for cases TRANS_LEADER_SEND_TO_PARALLEL and TRANS_PARALLEL_APPLY. For
other cases, it is set by stream_stop_internal. I think it would be
better to set the state explicitly for all cases to make the code look
consistent and remove it from stream_stop_internal(). The other reason
to remove setting the state from stream_stop_internal() is that when
that function is invoked from other places like
apply_handle_stream_commit(), it seems to be setting the idle before
actually we reach the idle state.

4. Apart from the above, I have made a few changes in the comments,
see attached.

-- 
With Regards,
Amit Kapila.


changes_amit_1.patch
Description: Binary data

Re: On login trigger: take three

[Ted Yu]

On Mon, Dec 19, 2022 at 1:40 AM Mikhail Gribkov  wrote:

> Hi Ted,
>
> > bq. in to the system
> > 'in to' -> into
> > bq. Any bugs in a trigger procedure
> > Any bugs -> Any bug
>
> Thanks!  Fixed typos in the attached v35.
>
> >   +   if (event == EVT_Login)
> >   +   dbgtag = CMDTAG_LOGIN;
> >   +   else
> >   +   dbgtag = CreateCommandTag(parsetree);
> > The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> It appears twice in fact, both cases are nearby: in the main code and
> under the assert-checking ifdef.
> Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
> function signature and ideology. CreateCommandTag finds a tag based on the
> command parse tree, while login event is a specific case when we do not
> have any command and the parse tree is NULL. Changing CreateCommandTag
> signature for these two calls looks a little bit overkill as it would lead
> to changing lots of other places the function is called from.
> We could move this snippet to a separate function, but here are the
> similar concerns I think: it would make sense for a more common or a more
> complex snippet, but not for two cases of if-else one-liners.
>
> --
>  best regards,
> Mikhail A. Gribkov
>
> e-mail: youzh...@gmail.com
> *http://www.flickr.com/photos/youzhick/albums
> *
> http://www.strava.com/athletes/5085772
> phone: +7(916)604-71-12
> Telegram: @youzhick
>
> Hi, Mikhail:
Thanks for the explanation.
It is Okay to keep the current formation of CMDTAG_LOGIN handling.

Cheers

Re: Inconsistency in reporting checkpointer stats

[Bharath Rupireddy]

On Fri, Dec 16, 2022 at 2:14 PM Kyotaro Horiguchi
 wrote:
>
> In the first place I don't like that we count the same things twice.
> Couldn't we count the number only by any one of them?
>
> If we remove CheckPointerStats.ckpt_bufs_written, CreateCheckPoint can
> get the final number as the difference between the start-end values of
> *the shared stats*. As long as a checkpoint runs on a single process,
> trace info in BufferSync will work fine.  Assuming single process
> checkpointing there must be no problem to do that. (Anyway the current
> shared stats update for checkpointer is assuming single-process).

What if someone resets checkpointer shared stats with
pg_stat_reset_shared()? In such a case, the checkpoint complete
message will not have the stats, no?

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

Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Bharath Rupireddy]

On Mon, Dec 19, 2022 at 5:22 PM Bharath Rupireddy
 wrote:
>
> On Mon, Dec 19, 2022 at 1:37 PM Michael Paquier  wrote:
> >
> > On Tue, Dec 13, 2022 at 09:32:19PM +0530, Bharath Rupireddy wrote:
> > > Okay, here's the v5 patch that I could come up with. It basically adds
> > > functions for dissecting WAL file names and computing offset from lsn.
> > > Thoughts?
> >
> > I had a second look at that, and I still have mixed feelings about the
> > addition of the SQL function, no real objection about
> > pg_dissect_walfile_name().
> >
> > I don't really think that we need a specific handling with a new
> > macro from xlog_internal.h that does its own parsing of the segment
> > number while XLogFromFileName() can do that based on the user input,
> > so I have simplified that.
> >
> > A second thing is the TLI that had better be returned as int8 and not
> > int4 so as we don't have a negative number for a TLI higher than 2B in
> > a WAL segment name.
>
> Thanks. The v6 patch LGTM.

A nitpick - can we also specify a use case for the function
pg_dissect_walfile_name(), that is, computing LSN from offset and WAL
file name, something like [1]?

[1]
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2f05b06f14..c36fcb83c8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -26110,7 +26110,17 @@ LOG:  Grand total: 1651920 bytes in 201
blocks; 622360 free (88 chunks); 1029560


 Extract the file sequence number and timeline ID from a WAL file
-name.
+name. This function is useful to compute LSN from a given offset
+and WAL file name, for example:
+
+postgres=# \set file_name '0001000100C000AB'
+postgres=# \set offset 256
+postgres=# SELECT '0/0'::pg_lsn + pd.segno * ps.setting::int +
:offset AS lsn FROM pg_dissect_walfile_name(:'file_name') pd,
pg_show_all_settings() ps WHERE ps.name = 'wal_segment_size';
+  lsn
+---
+ C001/AB000100
+(1 row)
+

   

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

Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Bharath Rupireddy]

On Mon, Dec 19, 2022 at 1:37 PM Michael Paquier  wrote:
>
> On Tue, Dec 13, 2022 at 09:32:19PM +0530, Bharath Rupireddy wrote:
> > Okay, here's the v5 patch that I could come up with. It basically adds
> > functions for dissecting WAL file names and computing offset from lsn.
> > Thoughts?
>
> I had a second look at that, and I still have mixed feelings about the
> addition of the SQL function, no real objection about
> pg_dissect_walfile_name().
>
> I don't really think that we need a specific handling with a new
> macro from xlog_internal.h that does its own parsing of the segment
> number while XLogFromFileName() can do that based on the user input,
> so I have simplified that.
>
> A second thing is the TLI that had better be returned as int8 and not
> int4 so as we don't have a negative number for a TLI higher than 2B in
> a WAL segment name.

Thanks. The v6 patch LGTM.

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

Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths

[Alvaro Herrera]

On 2022-Dec-02, Andres Freund wrote:

> Hi,
> 
> On 2022-12-02 14:22:55 +0900, Michael Paquier wrote:
> > On Fri, Nov 04, 2022 at 09:52:39AM +0900, Ian Lawrence Barwick wrote:
> > > CommitFest 2022-11 is currently underway, so if you are interested
> > > in moving this patch forward, now would be a good time to update it.
> > 
> > No replies after 4 weeks, so I have marked this entry as returned
> > with feedback.  I am still wondering what would be the best thing to
> > do here..
> 
> IMO this a bugfix, I don't think we can just close the entry, even if Matthias
> doesn't have time / energy to push it forward.

I have created one in the January commitfest,
https://commitfest.postgresql.org/41/
and rebased the patch on current master.  (I have not reviewed this.)

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
>From 385ccf1b7d68923009c73db493dbe74be34e305b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 19 Dec 2022 12:26:52 +0100
Subject: [PATCH v9] Add protections in xlog record APIs against large numbers
 and overflows.

Before this, it was possible for an extension to create malicious WAL records
that were too large to replay; or that would overflow the xl_tot_len field,
causing potential corruption in WAL record IO ops.

Emitting invalid records was also possible through pg_logical_emit_message(),
which allowed you to emit arbitrary amounts of data up to 2GB, much higher
than the replay limit of approximately 1GB.

This patch adds a limit to the size of an XLogRecord (1020MiB), checks
against overflows, and ensures that the reader infrastructure can read any
max-sized XLogRecords, such that the wal-generating backend will fail when
it attempts to insert unreadable records, instead of that insertion
succeeding but breaking any replication streams.

Author: Matthias van de Meent 
---
 src/backend/access/transam/xloginsert.c | 59 ++---
 src/include/access/xlogrecord.h | 13 ++
 2 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index f811523448..c25431f0e9 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -141,6 +141,17 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
 	   bool *topxid_included);
 static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
 	uint16 hole_length, char *dest, uint16 *dlen);
+static inline void XLogErrorDataLimitExceeded();
+
+/*
+ * Error due to exceeding the maximum size of a WAL record, or registering
+ * more datas than are being accounted for by the XLog infrastructure.
+ */
+static inline void
+XLogErrorDataLimitExceeded()
+{
+	elog(ERROR, "too much WAL data");
+}
 
 /*
  * Begin constructing a WAL record. This must be called before the
@@ -352,10 +363,25 @@ XLogRegisterData(char *data, uint32 len)
 {
 	XLogRecData *rdata;
 
-	Assert(begininsert_called);
+	Assert(begininsert_called && XLogRecordLengthIsValid(len));
+
+	/*
+	 * Check against max_rdatas; and ensure we don't fill a record with
+	 * more data than can be replayed. Records are allocated in one chunk
+	 * with some overhead, so ensure XLogRecordLengthIsValid() for that
+	 * size of record.
+	 *
+	 * Additionally, check that we don't accidentally overflow the
+	 * intermediate sum value on 32-bit systems by ensuring that the
+	 * sum of the two inputs is no less than one of the inputs.
+	 */
+	if (num_rdatas >= max_rdatas ||
+#if SIZEOF_SIZE_T == 4
+		 mainrdata_len + len < len ||
+#endif
+		!XLogRecordLengthIsValid((size_t) mainrdata_len + (size_t) len))
+		XLogErrorDataLimitExceeded();
 
-	if (num_rdatas >= max_rdatas)
-		elog(ERROR, "too much WAL data");
 	rdata = [num_rdatas++];
 
 	rdata->data = data;
@@ -391,7 +417,7 @@ XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
 	registered_buffer *regbuf;
 	XLogRecData *rdata;
 
-	Assert(begininsert_called);
+	Assert(begininsert_called && len <= UINT16_MAX);
 
 	/* find the registered buffer struct */
 	regbuf = _buffers[block_id];
@@ -400,14 +426,14 @@ XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
 			 block_id);
 
 	/*
-	 * Check against max_rdatas and ensure we do not register more data per
+	 * Check against max_rdatas; and ensure we don't register more data per
 	 * buffer than can be handled by the physical data format; i.e. that
 	 * regbuf->rdata_len does not grow beyond what
 	 * XLogRecordBlockHeader->data_length can hold.
 	 */
 	if (num_rdatas >= max_rdatas ||
 		regbuf->rdata_len + len > UINT16_MAX)
-		elog(ERROR, "too much WAL data");
+		XLogErrorDataLimitExceeded();
 
 	rdata = [num_rdatas++];
 
@@ -527,6 +553,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
    XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included)
 {
 	XLogRecData *rdt;
+	/*
+	 * Overflow of 

Re: Support logical replication of DDLs

[li jie]

I have presented some comments below:

1. AT_AddColumn

> + tmpobj = new_objtree_VA("ADD %{objtype}s %{definition}s", 3,
[ IF NOT EXISTS ] is missing here.

2.  AT_DropColumn
> + tmpobj = new_objtree_VA("DROP %{objtype}s %{column}I", 3,
[ IF EXISTS ] is missing here.

3. AT_DropConstraint
> + tmpobj = new_objtree_VA("DROP CONSTRAINT %{constraint}I", 2,
[ IF EXISTS ] is missing here.

4. AT_DetachPartition
> + tmpobj = new_objtree_VA("DETACH PARTITION %{partition_identity}D", 2,
[ CONCURRENTLY | FINALIZE ] is missing here.

5. deparse_CreateSeqStmt
> + ret = new_objtree_VA("CREATE %{persistence}s SEQUENCE %{identity}D 
> %{definition: }s", 3,
[ IF NOT EXISTS ] is missing here.

6. deparse_IndexStmt
> + ret = new_objtree_VA("CREATE %{unique}s INDEX %{concurrently}s 
> %{if_not_exists}s %{name}I ON %{table}D USING %{index_am}s (%{definition}s)", 
> 7,
[ INCLUDE ] and [ ONLY ] are  missing here.

7. deparse_RuleStmt
> + foreach(cell, actions)
> + list = lappend(list, new_string_object(lfirst(cell)));

if (actions == NIL)
 list = lappend(list, new_string_object("NOTHING"));
else
{
  foreach(cell, actions)
  list = lappend(list, new_string_object(lfirst(cell)));
}

8. AT_AddIndexConstraint
> + tmpobj = new_objtree_VA("ADD CONSTRAINT %{name}I %{constraint_type}s USING 
> INDEX %{index_name}I %{deferrable}s %{init_deferred}s", 6,
> + "type", ObjTypeString, "add constraint using index",
> + "name", ObjTypeString, get_constraint_name(constrOid),
> + "constraint_type", ObjTypeString,
> + istmt->deferrable ? "DEFERRABLE" : "NOT DEFERRABLE",

"constraint_type", ObjTypeString,
istmt->primary ? "PRIMARY KEY" : "UNIQUE",

9. regress test

Zheng Li  于2022年12月12日周一 12:58写道：


>
> Hi,
>
> Attached please find the DDL deparser testing module in the v45-0007
> patch, this testing module is written by Runqi Tian in [1] with minor
> modification from myself. I think we can
> start adding more tests to the module now that we're getting close to
> finish implementing the DDL deparser.
>
> This testing module ddl_deparse_regress aims to achieve the following
> four testing goals for the DDL deparser:
> 1. Test that the generated JSON blob is expected using SQL tests.
> 2. Test that the re-formed DDL command is expected using SQL tests.
> 3. Test that the re-formed DDL command has the same effect as the
> original command
>by comparing the results of pg_dump, using the SQL tests in 1 and 2.
> 4. Test that any new DDL syntax is handled by the DDL deparser by
> capturing and deparsing
>DDL commands ran by pg_regress.
>
> 1 and 2 is tested with SQL tests, by comparing the deparsed JSON blob
> and the re-formed command.
> 3 is tested with TAP framework in t/001_compare_dumped_results.pl
> 4 is tested with TAP framework and pg_regress in 002_regress_tests.pl,
> the execution is currently commented out because it will fail due
> unimplemented commands in the DDL deparser.
>
> [1] 
> https://www.postgresql.org/message-id/flat/CAH8n8_jMTunxxtP4L-3tc%3DGNamg%3Dmg1X%3DtgHr9CqqjjzFLwQng%40mail.gmail.com
>

The test patch is very useful.
 I see that the sql case in test_ddl_deparse_regress is similar to the
one in test_ddl_deparse.
Why don't we merge the test cases in test_ddl_deparse_regress into
test_ddl_deparse,
as the sql case can be completely reused with the sql files in test_ddl_deparse?
I believe this will make the tests more comprehensive and reduce redundancy.


Regards,
li jie

Re: Common function for percent placeholder replacement

[Alvaro Herrera]

On 2022-Dec-19, Peter Eisentraut wrote:

> On 14.12.22 18:05, Justin Pryzby wrote:
> > On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:
> > > + return replace_percent_placeholders(base_command, "df", (const char 
> > > *[]){target_detail, filename});
> > 
> > This is a "compound literal", which I gather is required by C99.
> > 
> > But I don't think that's currently being exercised, so I wonder if it's
> > going to break some BF members.
> 
> We already use this, for example in pg_dump.

Yeah, we have this

#define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}

which we then use like this

ArchiveEntry(fout,
 dbCatId,   /* catalog ID */
 dbDumpId,  /* dump ID */
 ARCHIVE_OPTS(.tag = datname,
  .owner = dba,
  .description = "DATABASE",
  .section = SECTION_PRE_DATA,
  .createStmt = creaQry->data,
  .dropStmt = delQry->data));

I think the new one is not great.  I wish we could do something more
straightforward, maybe like

  replace_percent_placeholders(base_command,
   PERCENT_OPT("f", filename),
   PERCENT_OPT("d", target_detail));

Is there a performance disadvantage to a variadic implementation?
Alternatively, have all these macro calls form an array.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
(Alexey Klyukin)

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Ted,

> bq. in to the system
> 'in to' -> into
> bq. Any bugs in a trigger procedure
> Any bugs -> Any bug

Thanks!  Fixed typos in the attached v35.

>   +   if (event == EVT_Login)
>   +   dbgtag = CMDTAG_LOGIN;
>   +   else
>   +   dbgtag = CreateCommandTag(parsetree);
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
can be moved into `CreateCommandTag`.

It appears twice in fact, both cases are nearby: in the main code and under
the assert-checking ifdef.
Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
function signature and ideology. CreateCommandTag finds a tag based on the
command parse tree, while login event is a specific case when we do not
have any command and the parse tree is NULL. Changing CreateCommandTag
signature for these two calls looks a little bit overkill as it would lead
to changing lots of other places the function is called from.
We could move this snippet to a separate function, but here are the similar
concerns I think: it would make sense for a more common or a more complex
snippet, but not for two cases of if-else one-liners.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Sat, Dec 17, 2022 at 3:29 PM Ted Yu  wrote:

>
>
> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov 
> wrote:
>
>> Hi Nikita,
>>
>> > Mikhail, I've checked the patch and previous discussion,
>> > the condition mentioned earlier is still actual:
>>
>> Thanks for pointing this out! My bad, I forgot to fix the documentation
>> example.
>> Attached v34 has this issue fixed, as well as a couple other problems
>> with the example code.
>>
>> --
>>  best regards,
>> Mikhail A. Gribkov
>>
> Hi,
>
> bq. in to the system
>
> 'in to' -> into
>
> bq. Any bugs in a trigger procedure
>
> Any bugs -> Any bug
>
> +   if (event == EVT_Login)
> +   dbgtag = CMDTAG_LOGIN;
> +   else
> +   dbgtag = CreateCommandTag(parsetree);
>
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> Cheers
>


v35-On_client_login_event_trigger.patch
Description: Binary data

Re: appendBinaryStringInfo stuff

[David Rowley]

On Mon, 19 Dec 2022 at 21:12, Andres Freund  wrote:
> Perhaps we should make appendStringInfoString() a static inline function
> - most compilers can compute strlen() of a constant string at compile
> time.

I had wondered about that, but last time I looked into it there was a
small increase in the size of the binary from doing it. Perhaps it
does not matter, but it's something to consider.

Re-thinking, I wonder if we could use the same macro trick used in
ereport_domain(). Something like:

#ifdef HAVE__BUILTIN_CONSTANT_P
#define appendStringInfoString(str, s) \
__builtin_constant_p(s) ? \
appendBinaryStringInfo(str, s, sizeof(s) - 1) : \
appendStringInfoStringInternal(str, s)
#else
#define appendStringInfoString(str, s) \
appendStringInfoStringInternal(str, s)
#endif

and rename the existing function to appendStringInfoStringInternal.

Because __builtin_constant_p is a known compile-time constant, it
should be folded to just call the corresponding function during
compilation.

Just looking at the binary sizes for postgres. I see:

unpatched = 9972128 bytes
inline function = 9990064 bytes
macro trick = 9984968 bytes

I'm currently not sure why the macro trick increases the binary at
all. I understand why the inline function does.

David

Re: Common function for percent placeholder replacement

[Peter Eisentraut]

On 14.12.22 18:05, Justin Pryzby wrote:

On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:

+   return replace_percent_placeholders(base_command, "df", (const char 
*[]){target_detail, filename});


This is a "compound literal", which I gather is required by C99.

But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.


We already use this, for example in pg_dump.

Re: Common function for percent placeholder replacement

[Peter Eisentraut]

On 14.12.22 17:09, Robert Haas wrote:

Well, OK, I'll tentatively cast a vote in favor of adopting
basebackup_to_shell's approach elsewhere. Or to put that in plain
English: I think that if the input appears to be malformed, it's
better to throw an error than to guess what the user meant. In the
case of basebackup_to_shell there are potentially security
ramifications to the setting involved so it seemed like a bad idea to
take a laissez faire approach. But also, just in general, if somebody
supplies an ssl_passphrase_command or archive_command with %, I don't really see why we should treat that differently
than trying to start the server with work_mem=banana.


I agree.  Here is an updated patch with the error checking included.
From 8bb85ad7ed99d0255ea82306b08ef7d343ab8edb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 19 Dec 2022 09:04:43 +0100
Subject: [PATCH v2] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.

Discussion: 
https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com
---
 .../basebackup_to_shell/basebackup_to_shell.c |  55 +---
 src/backend/access/transam/xlogarchive.c  |  45 +--
 src/backend/libpq/be-secure-common.c  |  38 ++
 src/backend/postmaster/shell_archive.c|  59 ++---
 src/common/Makefile   |   1 +
 src/common/archive.c  |  81 +---
 src/common/meson.build|   1 +
 src/common/percentrepl.c  | 120 ++
 src/include/common/percentrepl.h  |  18 +++
 9 files changed, 173 insertions(+), 245 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c 
b/contrib/basebackup_to_shell/basebackup_to_shell.c
index bdaf67a4c8..9a3d57c64b 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,7 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
const char *target_detail)
 {
-   StringInfoData buf;
-   const char *c;
-
-   initStringInfo();
-   for (c = base_command; *c != '\0'; ++c)
-   {
-   /* Anything other than '%' is copied verbatim. */
-   if (*c != '%')
-   {
-   appendStringInfoChar(, *c);
-   continue;
-   }
-
-   /* Any time we see '%' we eat the following character as well. 
*/
-   ++c;
-
-   /*
-* The following character determines what we insert here, or 
may
-* cause us to throw an error.
-*/
-   if (*c == '%')
-   {
-   /* '%%' is replaced by a single '%' */
-   appendStringInfoChar(, '%');
-   }
-   else if (*c == 'f')
-   {
-   /* '%f' is replaced by the filename */
-   appendStringInfoString(, filename);
-   }
-   else if (*c == 'd')
-   {
-   /* '%d' is replaced by the target detail */
-   appendStringInfoString(, target_detail);
-   }
-   else if (*c == '\0')
-   {
-   /* Incomplete escape sequence, expected a character 
afterward */
-   ereport(ERROR,
-   errcode(ERRCODE_SYNTAX_ERROR),
-   errmsg("shell command ends unexpectedly 
after escape character \"%%\""));
-   }
-   else
-   {
-   /* Unknown escape sequence */
-   ereport(ERROR,
-   errcode(ERRCODE_SYNTAX_ERROR),
-   errmsg("shell command contains 
unexpected escape sequence \"%c\"",
-  *c));
-   }
-   }
-
-   return buf.data;
+   return replace_percent_placeholders(base_command, "df", (const char 
*[]){target_detail, filename});
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c 

Re: appendBinaryStringInfo stuff

[Andres Freund]

Hi,

On 2022-12-19 07:13:40 +0100, Peter Eisentraut wrote:
> I found a couple of adjacent weird things:
> 
> There are a bunch of places in the json code that use
> appendBinaryStringInfo() where appendStringInfoString() could be used, e.g.,
> 
> appendBinaryStringInfo(buf, ".size()", 7);
> 
> Is there a reason for this?  Are we that stretched for performance?

strlen() isn't that cheap, so it doesn't generally seem unreasonable. I
don't think we should add the strlen overhead in places that can
conceivably be a bottleneck - and some of the jsonb code clearly can be
that.


> I find this kind of code very fragile.

But this is obviously an issue.

Perhaps we should make appendStringInfoString() a static inline function
- most compilers can compute strlen() of a constant string at compile
time.

Greetings,

Andres Freund

Re: (non) translatable string splicing

[Alvaro Herrera]

On 2022-Dec-16, Justin Pryzby wrote:

> I was surprised to see that this has been here for a few years (since
> 77517ba59) without complaints or inquiries from translators.

See https://postgr.es/m/13948.1501733...@sss.pgh.pa.us

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
http://smylers.hates-software.com/2005/09/08/1995c749.html

Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures

[Michael Paquier]

On Tue, Dec 13, 2022 at 09:32:19PM +0530, Bharath Rupireddy wrote:
> Okay, here's the v5 patch that I could come up with. It basically adds
> functions for dissecting WAL file names and computing offset from lsn.
> Thoughts?

I had a second look at that, and I still have mixed feelings about the
addition of the SQL function, no real objection about
pg_dissect_walfile_name().

I don't really think that we need a specific handling with a new
macro from xlog_internal.h that does its own parsing of the segment
number while XLogFromFileName() can do that based on the user input,
so I have simplified that.

A second thing is the TLI that had better be returned as int8 and not
int4 so as we don't have a negative number for a TLI higher than 2B in
a WAL segment name.
--
Michael
From 25fb0848cf3220a5fc9638c2cfd116e5390be887 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 19 Dec 2022 17:02:16 +0900
Subject: [PATCH v6] Add utility functions to disset WAL file name and compute
 offset

---
 src/include/catalog/pg_proc.dat  |  7 +++
 src/backend/access/transam/xlogfuncs.c   | 53 
 src/test/regress/expected/misc_functions.out | 18 +++
 src/test/regress/sql/misc_functions.sql  |  8 +++
 doc/src/sgml/func.sgml   | 16 ++
 5 files changed, 102 insertions(+)

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 719599649a..79cfd1378d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6365,6 +6365,13 @@
 { oid => '2851', descr => 'wal filename, given a wal location',
   proname => 'pg_walfile_name', prorettype => 'text', proargtypes => 'pg_lsn',
   prosrc => 'pg_walfile_name' },
+{ oid => '8205',
+  descr => 'sequence number and timeline ID given a wal filename',
+  proname => 'pg_dissect_walfile_name', prorettype => 'record',
+  proargtypes => 'text', proallargtypes => '{text,numeric,int8}',
+  provolatile => 's', proargmodes => '{i,o,o}',
+  proargnames => '{file_name,segno,timeline_id}',
+  prosrc => 'pg_dissect_walfile_name' },
 
 { oid => '3165', descr => 'difference in bytes, given two wal locations',
   proname => 'pg_wal_lsn_diff', prorettype => 'numeric',
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 487d5d9cac..0a31837ef1 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -432,6 +432,59 @@ pg_walfile_name(PG_FUNCTION_ARGS)
 	PG_RETURN_TEXT_P(cstring_to_text(xlogfilename));
 }
 
+/*
+ * Extract the sequence number and the timeline ID from given a WAL file
+ * name.
+ */
+Datum
+pg_dissect_walfile_name(PG_FUNCTION_ARGS)
+{
+#define PG_DISSECT_WALFILE_NAME_COLS 2
+	char	   *fname = text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *fname_upper;
+	char	   *p;
+	TimeLineID	tli;
+	XLogSegNo	segno;
+	Datum		values[PG_DISSECT_WALFILE_NAME_COLS] = {0};
+	bool		isnull[PG_DISSECT_WALFILE_NAME_COLS] = {0};
+	TupleDesc	tupdesc;
+	HeapTuple	tuple;
+	char		buf[256];
+	Datum		result;
+
+	fname_upper = pstrdup(fname);
+
+	/* Capitalize WAL file name. */
+	for (p = fname_upper; *p; p++)
+		*p = pg_toupper((unsigned char) *p);
+
+	if (!IsXLogFileName(fname_upper))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid WAL file name \"%s\"", fname)));
+
+	XLogFromFileName(fname_upper, , , wal_segment_size);
+
+	if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, "return type must be a row type");
+
+	/* Convert to numeric. */
+	snprintf(buf, sizeof buf, UINT64_FORMAT, segno);
+	values[0] = DirectFunctionCall3(numeric_in,
+	CStringGetDatum(buf),
+	ObjectIdGetDatum(0),
+	Int32GetDatum(-1));
+
+	values[1] = Int64GetDatum(tli);
+
+	tuple = heap_form_tuple(tupdesc, values, isnull);
+	result = HeapTupleGetDatum(tuple);
+
+	PG_RETURN_DATUM(result);
+
+#undef PG_DISSECT_WALFILE_NAME_COLS
+}
+
 /*
  * pg_wal_replay_pause - Request to pause recovery
  *
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 88bb696ded..fb676e9971 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -619,3 +619,21 @@ SELECT count(*) > 0 AS ok FROM pg_control_system();
  t
 (1 row)
 
+-- pg_dissect_walfile_name
+SELECT segno > 0 AS ok_segno, timeline_id
+  FROM pg_dissect_walfile_name('invalid');
+ERROR:  invalid WAL file name "invalid"
+SELECT segno > 0 AS ok_segno, timeline_id
+  FROM pg_dissect_walfile_name('00010001');
+ ok_segno | timeline_id 
+--+-
+ t|   1
+(1 row)
+
+SELECT segno > 0 AS ok_segno, timeline_id
+  FROM pg_dissect_walfile_name('000100af');
+ ok_segno | timeline_id 
+--+-
+ t|  4294967295
+(1 row)
+
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index