RE: Improve EXPLAIN output for multicolumn B-Tree Index

[Masahiro.Ikeda]

>>=# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id3 = 101;
>>                                                          QUERY PLAN          
>>                                                 
>>---
>> Index Scan using test_idx on ikedamsh.test  (cost=0.42..12630.10 rows=1 
>>width=18) (actual time=0.175..279.819 rows=1 loops=1)
>>   Output: id1, id2, id3, value
>>   Index Cond: (test.id1 = 1)                 -- Change the output. Show only 
>>the bound quals. 
>>   Index Filter: (test.id3 = 101)              -- New. Output quals which are 
>>not used as the bound quals
>>   Rows Removed by Index Filter: 49    -- New. Output when ANALYZE option 
>>is specified
>> Planning Time: 0.354 ms
>> Execution Time: 279.908 ms
>> (7 rows)
>
> I don't think we want to split these clauses. Index Cond should indicate the 
> conditions applied
> to the index scan. Bound quals should be listed separately even though they 
> will have an
> intersection with Index Cond. I am not sure whether Index Filter is the right 
> name, 
> maybe Index Bound Cond: But I don't know this area enough to make a final 
> call.

OK, I understood that it's better to only add new ones. I think "Index Filter" 
fits other than "Index
Bound Cond" if we introduce "Rows Removed By Index Filter".

> About Rows Removed by Index Filter: it's good to provide a number when 
> ANALYZE is
> specified, but it will be also better to specify what was estimated. We do 
> that for (cost snd rows etc.)
> but doing that somewhere in the plan output may not have a precedent. I think 
> we should try that
> and see what others think.

It's interesting! It’s an idea that can be applied not only to multi-column 
indexes, right?
I will consider the implementation and discuss it in a new thread. However, I 
would like to
focus on the feature to output information about multi-column indexes at first.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Backporting BackgroundPsql

[Heikki Linnakangas]

On 26/06/2024 03:25, Michael Paquier wrote:

On Wed, Jun 26, 2024 at 02:12:42AM +0200, Alvaro Herrera wrote:

FWIW I successfully used the preliminary PqFFI stuff Andrew posted to
write a test program for bug #18377, which I think ended up being better
than with BackgroundPsql, so I think it's a good way forward.  As for
back-patching it, I suspect we're going to end up backpatching the
framework anyway just because we'll want to have it available for
backpatching future tests, even if we keep a backpatch minimal by doing
only the framework and not existing tests.

I also backpatched the PqFFI and PostgreSQL::Session modules to older PG
branches, to run my test program there.  This required only removing
some lines from PqFFI.pm that were about importing libpq functions that
older libpq didn't have.


Nice!  I definitely +1 the backpatching of the testing bits.  This
stuff can make validating bugs so much easier, particularly when there
are conflicting parts in the backend after a cherry-pick.


I haven't looked closely at the new PgFFI stuff but +1 on that in 
general, and it makes sense to backport that once it lands on master. In 
the meanwhile, I think we should backport BackgroundPsql as it is, to 
make it possible to backport tests using it right now, even if it is 
short-lived.


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

RE: Improve EXPLAIN output for multicolumn B-Tree Index

[Masahiro.Ikeda]

> On Mon, 24 Jun 2024 at 14:42, Jelte Fennema-Nio  wrote:
> >
> > On Mon, 24 Jun 2024 at 13:02, Matthias van de Meent
> >  wrote:
> > > It does not really behave similar: index scan keys (such as the
> > > id3=101 scankey) don't require visibility checks in the btree code,
> > > while the Filter condition _does_ require a visibility check, and
> > > delegates the check to the table AM if the scan isn't Index-Only, or
> > > if the VM didn't show all-visible during the check.
> >
> > Any chance you could point me in the right direction for the
> > code/docs/comment about this? I'd like to learn a bit more about why
> > that is the case, because I didn't realize visibility checks worked
> > differently for index scan keys and Filter keys.
> 
> This can be derived by combining how Filter works (it only filters the 
> returned live tuples)
> and how Index-Only scans work (return the index tuple, unless !ALL_VISIBLE, 
> in which
> case the heap tuple is projected). There have been several threads more or 
> less
> recently that also touch this topic and closely related topics, e.g. [0][1].

Thanks! I could understand what is difference between INCLUDE based filter and 
index filter.

> > > As you can see, there's a huge difference in performance. Putting
> > > both non-bound and "normal" filter clauses in the same Filter clause
> > > will make it more difficult to explain performance issues based on
> > > only the explain output.
> >
> > Fair enough, that's of course the main point of this patch in the
> > first place: being able to better interpret the explain plan when you
> > don't have access to the schema. Still I think Filter is the correct
> > keyword for both, so how about we make it less confusing by making the
> > current "Filter" more specific by calling it something like "Non-key
> > Filter" or "INCLUDE Filter" and then call the other something like
> > "Index Filter" or "Secondary Bound Filter".
> 
> I'm not sure how debuggable explain plans are without access to the schema, 
> especially
> when VERBOSE isn't configured, so I would be hesitant to accept that as an 
> argument
> here.

IMHO, it's nice to be able to understand the differences between each
FILTER even without the VERBOSE option. (+1 for Jelte Fennema-Nio's idea)

Even without access to the schema, it would be possible to quickly know if
the plan is not as expected, and I believe there are virtually no disadvantages
to having multiple "XXX FILTER" outputs.

If it's better to output such information only with the VERBOSE option, 
What do you think about the following idea?
* When the VERBOSE option is not specified, output as "Filter" in all cases
* When the VERBOSE option is specified, output as "Non-key Filter", "INCLUDE 
Filter" 
  and "Index Filter".

In addition, I think it would be good to mention the differences between each 
filter in 
the documentation.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Proposal: Document ABI Compatibility

[Laurenz Albe]

On Tue, 2024-06-25 at 13:55 -0400, David E. Wheeler wrote:
> On Jun 25, 2024, at 7:33 AM, Peter Eisentraut  wrote:
> 
> > I took at a stab at this, using some of your text, but discussing API and 
> > ABI separately.
> 
> Oh man this is fantastic, thank you! I’d be more than happy to just turn this 
> into a patch.
> But where should it go? Upthread I assumed xfunc.sgml, and still think that’s 
> a likely
> candidate. Perhaps I’ll just start there --- unless someone thinks it should 
> go somewhere
> other than the docs.

Perhaps such information should go somewhere here:
https://www.postgresql.org/support/versioning/

Yours,
Laurenz Albe

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

On Sat, 2024-06-22 at 20:13 -0700, David G. Johnston wrote:
> [bikeshedding discussion about SQL syntax]

Sure, something like CREATE TRIGGER ... USING {INVOKER|CURRENT} ROLE
orsimilar would work, but think that this discussion is premature
at this point.  If we have syntax to specify the behavior
of deferred triggers, that needs a new column in "pg_trigger", support
in pg_get_triggerdef(), pg_dump, pg_upgrade etc.

All that is possible, but keep in mind that we are talking about corner
case behavior.  To the best of my knowledge, nobody has even noticed the
difference in behavior up to now.

I think that we should have some consensus about the following before
we discuss syntax:

- Does anybody depend on the current behavior and would be hurt if
  my current patch went in as it is?

- Is this worth changing at all or had we better document the current
  behavior and leave it as it is?

Concerning the latter, I am hoping for a detailed description of our
customer's use case some time soon.

Yours,
Laurenz Albe

Re: Conflict Detection and Resolution

[Amit Kapila]

On Tue, Jun 25, 2024 at 3:39 PM shveta malik  wrote:
>
> On Tue, Jun 25, 2024 at 3:12 PM Amit Kapila  wrote:
> >
> > On Mon, Jun 24, 2024 at 1:47 PM shveta malik  wrote:
> > >
> > > On Thu, Jun 20, 2024 at 6:41 PM Amit Kapila  
> > > wrote:
> > > >
> > > > >> In the second patch, we can implement simple built-in resolution
> > > > >> strategies like apply and skip (which can be named as remote_apply 
> > > > >> and
> > > > >> keep_local, see [3][4] for details on these strategies) with ERROR or
> > > > >> LOG being the default strategy. We can allow these strategies to be
> > > > >> configured at the global and table level.
> > >
> > > Before we implement resolvers, we need a way to configure them. Please
> > > find the patch002 which attempts to implement Global Level Conflict
> > > Resolvers Configuration.  Note that patch002 is dependent upon
> > > Conflict-Detection patch001 which is reviewed in another thread [1].
> > > I have attached patch001 here for convenience and to avoid CFBot
> > > failures. But please use [1] if you have any comments on patch001.
> > >
> > > New DDL commands in patch002 are:
> > >
> > > To set global resolver for given conflcit_type:
> > > SET CONFLICT RESOLVER 'conflict_resolver' FOR 'conflict_type'
> > >
> > > To reset to default resolver:
> > > RESET CONFLICT RESOLVER FOR 'conflict_type'
> > >
> >
> > Does setting up resolvers have any meaning without subscriptions? I am
> > wondering whether we should allow to set up the resolvers at the
> > subscription level. One benefit is that users don't need to use a
> > different DDL to set up resolvers. The first patch gives a conflict
> > detection option at the subscription level, so it would be symmetrical
> > to provide a resolver at the subscription level. Yet another benefit
> > could be that it provides users facility to configure different
> > resolvers for a set of tables belonging to a particular
> > publication/node.
>
> There can be multiple tables included in a publication with varying
> business use-cases and thus may need different resolvers set, even
> though they all are part of the same publication.
>

Agreed but this is the reason we are planning to keep resolvers at the
table level. Here, I am asking to set resolvers at the subscription
level rather than at the global level.

> > >
> > > 
> > >
> > > As suggested in [2] and above, it seems logical to have table-specific
> > > resolvers configuration along with global one.
> > >
> > > Here is the proposal for table level resolvers:
> > >
> > > 1) We can provide support for table level resolvers using ALTER TABLE:
> > >
> > > ALTER TABLE  SET CONFLICT RESOLVER  on ,
> > >SET CONFLICT RESOLVER
> > >  on , ...;
> > >
> > > Reset can be done using:
> > > ALTER TABLE  RESET CONFLICT RESOLVER on ,
> > >   RESET CONFLICT RESOLVER on
> > > , ...;
> > >
> > > Above commands will save/remove configuration in/from the new system
> > > catalog pg_conflict_rel.
> > >
> > > 2) Table level configuration (if any) will be given preference over
> > > global ones. The tables not having table-specific resolvers will use
> > > global configured ones.
> > >
> > > 3) If the table is a partition table, then resolvers created for the
> > > parent will be inherited by all child partition tables. Multiple
> > > resolver entries will be created, one for each child partition in the
> > > system catalog (similar to constraints).
> > >
> > > 4) Users can also configure explicit resolvers for child partitions.
> > > In such a case, child's resolvers will override inherited resolvers
> > > (if any).
> > >
> > > 5) Any attempt to RESET (remove) inherited resolvers on the child
> > > partition table *alone* will result in error:  "cannot reset inherited
> > > resolvers" (similar to constraints). But RESET of explicit created
> > > resolvers (non-inherited ones) will be permitted for child partitions.
> > > On RESET, the resolver configuration will not fallback to the
> > > inherited resolver again. Users need to explicitly configure new
> > > resolvers for the child partition tables (after RESET) if needed.
> > >
> >
> > Why so? If we can allow the RESET command to fallback to the inherited
> > resolver it would make the behavior consistent for the child table
> > where we don't have performed SET.
>
> Thought behind not making it fallback is since the user has done
> 'RESET', he may want to remove the resolver completely. We don't know
> if he really wants to go back to the previous one. If he does, it is
> easy to set it again. But if he does not, and we set the inherited
> resolver again during 'RESET', there is no way he can drop that
> inherited resolver alone on the child partition.
>

I see your point but normally RESET allows us to go back to the
default which in this case would be the resolver inherited from the
parent table.

-- 
With Regards,
Amit Kapila.

Re: Showing applied extended statistics in explain Part 2

[Tatsuro Yamada]

Hi Tomas!

Thanks for the comments!

1) The patch is not added to the CF app, which I think is a mistake. Can
> you please add it to the 2024-07 commitfest? Otherwise people may not be
> aware of it, won't do reviews etc. It'll require posting a rebased
> patch, but should not be a big deal.
>

I added the patch to the 2024-07 commitfest today.


2) Not having the patch in a CF also means cfbot is not running tests on
> it. Which is unfortunate, because the patch actually has an a bug cfbot
> would find - I've noticed it after running the tests through the github
> CI, see [2].
> 3) The bug has this symptom:
>   ERROR:  unrecognized node type: 268
>   CONTEXT:  PL/pgSQL function check_estimated_rows(text) line 7 ...
>   STATEMENT:  SELECT * FROM check_estimated_rows('SELECT a, b FROM ...
> 4) I can think of two basic ways to fix this issue - either allow
> copying of the StatisticExtInto node, or represent the information in a
> different way (e.g. add a new node for that purpose, or use existing
> nodes to do that).
>

Thanks for the info. I'll investigate using cfbot.
To fix the problem, I understand we need to create a new struct like
(statistics OID, list of clauses, flag is_or).


5) In [3] Tom raised two possible issues with doing this - cost of
> copying the information, and locking. For the performance concerns, I
> think the first thing we should do is measuring how expensive it is. I
> suggest measuring the overhead for about three basic cases:
>

Okay, I'll measure it once the patch is completed and check the overhead.
I read [3][4] and in my opinion I agree with Robert.
As with indexes, there should be a mechanism for determining whether
extended statistics are used or not. If it were available, users would be
able to
tune using extended statistics and get better execution plans.



> 6) I'm not sure we want to have this under EXPLAIN (VERBOSE). It's what
> I did in the initial PoC patch, but maybe we should invent a new flag
> for this purpose, otherwise VERBOSE will cover too much stuff? I'm
> thinking about "STATS" for example.
>
> This would probably mean the patch should also add a new auto_explain
> "log_" flag to enable/disable this.
>

I thought it might be better to do this, so I'll fix it.



> 7) The patch really needs some docs - I'd mention this in the EXPLAIN
> docs, probably. There's also a chapter about estimates, maybe that
> should mention this too? Try searching for places in the SGML docs
> mentioning extended stats and/or explain, I guess.
>

I plan to create documentation after the specifications are finalized.



> For tests, I guess stats_ext is the right place to test this. I'm not
> sure what's the best way to do this, though. If it's covered by VERBOSE,
> that seems it might be unstable - and that would be an issue. But maybe
> we might add a function similar to check_estimated_rows(), but verifying
>  the query used the expected statistics to estimate expected clauses.
>

As for testing, I think it's more convenient for reviewers to include it in
the patch,
so I'm thinking of including it in the next patch.



So there's stuff to do to make this committable, but hopefully this
> review gives you some guidance regarding what/how ;-)
>

Thank you! It helps me a lot!

The attached patch does not correspond to the above comment.
But it does solve some of the issues mentioned in previous threads.

The next patch is planned to include:
6) Add stats option to explain command
8) Add regression test (stats_ext.sql)
4) Add new node (resolve errors in cfbot and prepared statement)

Regards,
Tatsuro Yamada



> [1]
>
> https://www.postgresql.org/message-id/TYYPR01MB82310B308BA8770838F681619E5E2%40TYYPR01MB8231.jpnprd01.prod.outlook.com
>
> [2] https://cirrus-ci.com/build/6436352672137216
>
> [3]
> https://www.postgresql.org/message-id/459863.1627419001%40sss.pgh.pa.us
>
> [4]
>
> https://www.postgresql.org/message-id/CA%2BTgmoZU34zo4%3DhyqgLH16iGpHQ6%2BQAesp7k5a1cfZB%3D%2B9xtsw%40mail.gmail.com
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
>


0001-show-stats-in-explain-rebased-on-15c9ac36.patch
Description: Binary data

Re: pg_createsubscriber: drop pre-existing subscriptions from the converted node

[vignesh C]

On Fri, 21 Jun 2024 at 16:51, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Hackers,
>
> This is a follow-up thread for pg_createsubscriber [1]. I started a new thread
> since there is no activity around here.
>
> ## Problem
>
> Assuming that there is a cascading replication like below:
>
> node A --(logical replication)--> node B --(streaming replication)--> node C
>
> In this case, subscriptions exist even on node C, but it does not try to 
> connect
> to node A because the logical replication launcher/worker won't be launched.
> After the conversion, node C becomes a subscriber for node B, and the 
> subscription
> toward node A remains. Therefore, another worker that tries to connect to 
> node A
> will be launched, raising an ERROR [2]. This failure may occur even during the
> conversion.
>
> ## Solution
>
> The easiest solution is to drop pre-existing subscriptions from the converted 
> node.
> To avoid establishing connections during the conversion, slot_name is set to 
> NONE
> on the primary first, then drop on the standby. The setting will be restored 
> on the
> primary node.
> Attached patch implements the idea. Test script is also included, but not 
> sure it should
> be on the HEAD

Few comments:
1) Should we do this only for the enabled subscription, otherwise the
disabled subscriptions will be enabled after running
pg_createsubscriber:
+obtain_and_disable_pre_existing_subscriptions(struct LogicalRepInfo *dbinfo)
+{
+   PQExpBuffer query = createPQExpBuffer();
+
+   for (int i = 0; i < num_dbs; i++)
+   {
+   PGconn *conn;
+   PGresult   *res;
+   int ntups;
+
+   /* Connect to publisher */
+   conn = connect_database(dbinfo[i].pubconninfo, true);
+
+   appendPQExpBuffer(query,
+ "SELECT s.subname,
s.subslotname FROM pg_catalog.pg_subscription s "
+ "INNER JOIN
pg_catalog.pg_database d ON (s.subdbid = d.oid) "
+ "WHERE d.datname = '%s'",
+ dbinfo[i].dbname);
+

2) disconnect_database not called here, should the connection be disconnected:
+drop_pre_existing_subscriptions(struct LogicalRepInfo *dbinfo)
+{
+   PQExpBuffer query = createPQExpBuffer();
+
+   for (int i = 0; i < num_dbs; i++)
+   {
+   PGconn   *conn;
+   struct LogicalRepInfo info = dbinfo[i];
+
+   /* Connect to subscriber */
+   conn = connect_database(info.subconninfo, false);
+
+   for (int j = 0; j < info.num_subscriptions; j++)
+   {
+   appendPQExpBuffer(query,
+ "DROP
SUBSCRIPTION %s;", info.pre_subnames[j]);
+   PQexec(conn, query->data);
+   resetPQExpBuffer(query);
+   }
+   }

3) Similarly here too:
+static void
+enable_subscirptions_on_publisher(struct LogicalRepInfo *dbinfo)
+{
+   PQExpBuffer query = createPQExpBuffer();
+
+   for (int i = 0; i < num_dbs; i++)
+   {
+   PGconn   *conn;
+   struct LogicalRepInfo info = dbinfo[i];
+
+   /* Connect to publisher */
+   conn = connect_database(info.pubconninfo, false);

4) them should be then here:
+   /* ...and them enable the subscription */
+   appendPQExpBuffer(query,
+ "ALTER
SUBSCRIPTION %s ENABLE",
+ info.pre_subnames[j]);
+   PQclear(PQexec(conn, query->data));
+   resetPQExpBuffer(query);


> BTW, I found that LogicalRepInfo.oid won't be used. If needed, I can create
> another patch to remove the attribute.

I was able to compile without this, I think this can be removed.

Regards,
Vignesh

Re: Logical Replication of sequences

[Peter Smith]

Here are my initial review comments for the first patch v20240625-0001.

==
General

1. Missing docs?

Section 9.17. "Sequence Manipulation Functions" [1] describes some
functions. Shouldn't your new function be documented here also?

~~~

2. Missing tests?

Shouldn't there be some test code that at least executes your new
pg_sequence_state function to verify that sane values are returned?

==
Commit Message

3.
This patch introduces new functionalities to PostgreSQL:
- pg_sequence_state allows retrieval of sequence values using LSN.
- SetSequence enables updating sequences with user-specified values.

~

3a.
I didn't understand why this says "using LSN" because IIUC 'lsn' is an
output parameter of that function. Don't you mean "... retrieval of
sequence values including LSN"?

~

3b.
Does "user-specified" make sense? Is this going to be exposed to a
user? How about just "specified"?

==
src/backend/commands/sequence.c

4. SetSequence:

+void
+SetSequence(Oid seq_relid, int64 value)

Would 'new_last_value' be a better parameter name here?

~~~

5.
This new function logic looks pretty similar to the do_setval()
function. Can you explain (maybe in the function comment) some info
about how and why it differs from that other function?

~~~

6.
I saw that RelationNeedsWAL() is called 2 times. It may make no sense,
but is it possible to assign that to a variable 1st time so you don't
need to call it 2nd time within the critical section?

~~~

NITPICK - remove junk (') char in comment

NITPICK - missing periods (.) in multi-sentence comment

~~~

7.
-read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
+read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple,
+XLogRecPtr *lsn)

7a.
The existing parameters were described in the function comment. So,
the new 'lsn' parameter should be described here also.

~

7b.
Maybe the new parameter name should be 'lsn_res' or 'lsn_out' or
similar to emphasise that this is a returned value.

~~

NITPICK - tweaked comment. YMMV.

~~~

8. pg_sequence_state:

Should you give descriptions of the output parameters in the function
header comment? Otherwise, where are they described so called knows
what they mean?

~~~

NITPICK - /relid/seq_relid/

NITPICK - declare the variables in the same order as the output parameters

NITPICK - An alternative to the memset for nulls is just to use static
initialisation
"bool nulls[4] = {false, false, false, false};"

==
+extern void SetSequence(Oid seq_relid, int64 value);

9.
Would 'SetSequenceLastValue' be a better name for what this function is doing?

==

99.
See also my attached diff which is a top-up patch implementing those
nitpicks mentioned above. Please apply any of these that you agree
with.

==
[1] https://www.postgresql.org/docs/devel/functions-sequence.html

Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 57453a7..9bad121 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -349,7 +349,7 @@ SetSequence(Oid seq_relid, int64 value)
/* open and lock sequence */
init_sequence(seq_relid, &elm, &seqrel);
 
-   /* lock page' buffer and read tuple */
+   /* lock page buffer and read tuple */
seq = read_seq_tuple(seqrel, &buf, &seqdatatuple, NULL);
 
/* check the comment above nextval_internal()'s equivalent call. */
@@ -397,8 +397,10 @@ SetSequence(Oid seq_relid, int64 value)
 
UnlockReleaseBuffer(buf);
 
-   /* Clear local cache so that we don't think we have cached numbers */
-   /* Note that we do not change the currval() state */
+   /*
+* Clear local cache so that we don't think we have cached numbers.
+* Note that we do not change the currval() state.
+*/
elm->cached = elm->last;
 
relation_close(seqrel, NoLock);
@@ -1275,8 +1277,9 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple 
seqdatatuple,
 RelationGetRelationName(rel), sm->magic);
 
/*
-* If the caller requested it, set the page LSN. This allows deciding
-* which sequence changes are before/after the returned sequence state.
+* If the caller requested it, return the page LSN. This allows the
+* caller to determine which sequence changes are before/after the
+* returned sequence state.
 */
if (lsn)
*lsn = PageGetLSN(page);
@@ -1912,7 +1915,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
 Datum
 pg_sequence_state(PG_FUNCTION_ARGS)
 {
-   Oid relid = PG_GETARG_OID(0);
+   Oid seq_relid = PG_GETARG_OID(0);
SeqTableelm;
Relationseqrel;
Buffer  buf;
@@ -1920,21 +1923,21 @@ pg_sequence_state(PG_FUNCTION_ARGS)
Form_pg_sequence_data seq;
Datum   result;
 
+   XLogRecPtr  lsn;
int64

RE: New standby_slot_names GUC in PG 17

[Zhijie Hou (Fujitsu)]

On Wednesday, June 26, 2024 12:49 PM Bertrand Drouvot 
 wrote:
> 
> Hi,
> 
> On Wed, Jun 26, 2024 at 04:17:45AM +, Zhijie Hou (Fujitsu) wrote:
> > On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada
>  wrote:
> > >
> > > On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila
> > > 
> > > wrote:
> > > >
> > > > I feel synchronized better indicates the purpose because we ensure
> > > > such slots are synchronized before we process changes for logical
> > > > failover slots. We already have a 'failover' option for logical
> > > > slots which could make things confusing if we add 'failover' where
> > > > physical slots need to be specified.
> > >
> > > Agreed. So +1 for synchronized_stnadby_slots.
> >
> > +1.
> >
> > Since there is a consensus on this name, I am attaching the patch to
> > rename the GUC to synchronized_stnadby_slots. I have confirmed that
> > the regression tests and pgindent passed for the patch.
> A few comments:

Thanks for the comments!

> 1 
> 
> In the commit message:
> 
> "
> The standby_slot_names GUC is intended to allow specification of physical
> standby slots that must be synchronized before they are visible to
> subscribers
> "
> 
> Not sure that wording is correct, if we feel the need to explain the GUC, 
> maybe
> repeat some wording from bf279ddd1c?

I intentionally copied some words from release note of this GUC which was
also part of the content in the initial email of this thread. I think it
would be easy to understand than the original commit msg. But others may
have different opinion, so I would leave the decision to the committer. (I 
adjusted
a bit the word in this version).

> 
> 2 
> 
> Should we rename StandbySlotNamesConfigData too?
> 
> 3 
> 
> Should we rename SlotExistsInStandbySlotNames too?
> 
> 4 
> 
> Should we rename validate_standby_slots() too?
> 

Renamed these to the names suggested by Amit.

Attach the v2 patch set which addressed above and removed
the changes in release-17.sgml according to the comment from Amit.

Best Regards,
Hou zj


v2-0001-Rename-standby_slot_names-to-synchronized_standby.patch
Description:  v2-0001-Rename-standby_slot_names-to-synchronized_standby.patch

Re: walsender.c comment with no context is hard to understand

[vignesh C]

On Mon, 3 Jun 2024 at 11:21, Peter Smith  wrote:
>
> Hi,
>
> I was looking at this code comment and wondered what it meant. AFAICT
> over time code has been moved around causing comments to lose their
> original context, so now it is hard to understand what they are
> saying.
>
> ~~~
>
> After a 2017 patch [1] the code in walsender.c function
> logical_read_xlog_page() looked like this:
>
> /* make sure we have enough WAL available */
> flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
>
> /* fail if not (implies we are going to shut down) */
> if (flushptr < targetPagePtr + reqLen)
> return -1;
>
> ~~~
>
> The same code in HEAD now looks like this:
>
> /*
> * Make sure we have enough WAL available before retrieving the current
> * timeline. This is needed to determine am_cascading_walsender accurately
> * which is needed to determine the current timeline.
> */
> flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
>
> /*
> * Since logical decoding is also permitted on a standby server, we need
> * to check if the server is in recovery to decide how to get the current
> * timeline ID (so that it also cover the promotion or timeline change
> * cases).
> */
> am_cascading_walsender = RecoveryInProgress();
>
> if (am_cascading_walsender)
> GetXLogReplayRecPtr(&currTLI);
> else
> currTLI = GetWALInsertionTimeLine();
>
> XLogReadDetermineTimeline(state, targetPagePtr, reqLen, currTLI);
> sendTimeLineIsHistoric = (state->currTLI != currTLI);
> sendTimeLine = state->currTLI;
> sendTimeLineValidUpto = state->currTLIValidUntil;
> sendTimeLineNextTLI = state->nextTLI;
>
> /* fail if not (implies we are going to shut down) */
> if (flushptr < targetPagePtr + reqLen)
> return -1;
>
> ~~~
>
> Notice how the "fail if not" comment has become distantly separated
> from the flushptr assignment it was once adjacent to, so that comment
> hardly makes sense anymore -- e.g. "fail if not" WHAT?
>
> Perhaps the comment should say something like it used to:
> /* Fail if there is not enough WAL available. This can happen during
> shutdown. */

Agree with this, +1 for this change.

Regards,
Vignesh

Re: RFC: Additional Directory for Extensions

[Jelte Fennema-Nio]

On Wed, 26 Jun 2024 at 00:32, David E. Wheeler  wrote:
> In other news, here’s an updated patch that expands the documentation to 
> record that the destination directory is a prefix, and full paths should be 
> used under it. Also take the opportunity to document the PGXS DESTDIR 
> variable as the thing to use to install files under the destination directory.

Docs are much clearer now thanks.

full = substitute_libpath_macro(name);
+   /*
+* If extension_destdir is set, try to find the file there first
+*/
+   if (*extension_destdir != '\0')
+   {
+   full2 = psprintf("%s%s", extension_destdir, full);
+   if (pg_file_exists(full2))
+   {
+   pfree(full);
+   return full2;
+   }
+   pfree(full2);
+   }

I think this should be done differently. For two reasons:
1. I don't think extension_destdir should be searched when $libdir is
not part of the name.
2. find_in_dynamic_libpath currently doesn't use extension_destdir at
all, so if there is no slash in the filename we do not search
extension_destdir.

I feel like changing the substitute_libpath_macro function a bit (or
adding a new similar function) is probably the best way to achieve
that.

We should also check somewhere (probably GUC check hook) that
extension_destdir is an absolute path.

> It still requires a server restart;

When reading the code I see no reason why this cannot be PGC_SIGHUP.
Even though it's probably not needed to change on a running server, I
think it's better to allow that. Even just so people can disable it if
necessary for some reason without restarting the process.

> I can change it back to superuser-only if that’s the consensus.

It still is GUC_SUPERUSER_ONLY, right?

> For those who prefer a GitHub patch review experience, see this PR:
>
>   https://github.com/theory/postgres/pull/3/files

Sidenote: The "D" link for each patch on cfbot[1] now gives a similar
link for all commitfest entries[2].

[1]: http://cfbot.cputube.org/
[2]: https://github.com/postgresql-cfbot/postgresql/compare/cf/4913~1...cf/4913

Re: RFC: Additional Directory for Extensions

[Jelte Fennema-Nio]

On Tue, 25 Jun 2024 at 19:33, David E. Wheeler  wrote:
>
> On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio  wrote:
>
> > Still, for the sake of completeness it might make sense to support
> > this whole list in extension_destdir. (assuming it's easy to do)
>
> It should be with the current patch, which just uses a prefix to paths in 
> `pg_config`.

Ah alright, I think it confused me because I never saw bindir being
used. But as it turns out the current backend code never uses bindir.
So that makes sense. I guess to actually use the binaries from the
extension_destdir/$BINDIR the operator needs to set PATH accordingly,
or the extension needs to be changed to support extension_destdir.

It might be nice to add a helper function to find binaries in BINDIR,
now that the resolution logic is more complex. Even if postgres itself
doesn't use it. That would make it easier for extensions to be
modified to support extension_distdir. Something like
find_bindir_executable(char *name)

RE: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE

[Masahiro.Ikeda]

Hi,

Thanks for working it! I'm interested in this feature, so I'd like to 
participate in the
patch review. Though I've just started looking at the patch, I have two comments
about the v6 patch. (And I want to confirm the thread active.)


1) Unify the print format of leader and worker

In show_tidbitmap_info(), the number of exact/loosy blocks of the leader and 
workers
are printed. I think the printed format should be same. Currently, the leader 
does not
print the blocks of exact/lossy with a value of 0, but the workers could even 
if it is 0.

IMHO, it's better to print both exact/lossy blocks if at least one of the 
numbers of
exact/lossy blocks is greater than 0. After all, the print logic is redundant 
for leader
and workers, but I thought it would be better to make it a common function.

2) Move es->workers_state check

In show_tidbitmap_info(), ExplainOpenWorker() and ExplainCloseWorker() are 
called
after checking es->worker_state is not NULL. However, es->workers_state seem to 
be
able to be NULL only for the Gather node (I see ExplainPrintPlan()). Also, 
reading the
comments, there is a description that each worker information needs to be hidden
when printing the plan.

Even if es->workers_state becomes NULL in BitmapHeapScan node in the future,
I think that workers' information(Heap Blocks) should not be printed. Therefore,
I think es->workers_state check should be move to the place of 
"if (planstate->pstate != NULL)" like ExplainNode(), doesn't it?

IIUC, we need to correct show_sort_info() and so on too…

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Avoid orphaned objects dependencies, take 3

[Bertrand Drouvot]

Hi,

On Fri, Jun 21, 2024 at 01:22:43PM +, Bertrand Drouvot wrote:
> Another thought for the RelationRelationId class case: we could check if there
> is a lock first and if there is no lock then acquire one. That way that would
> ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> add "unecessary" locking (see 2. mentioned previously).

Please find attached v12 implementing this idea for the RelationRelationId class
case. As mentioned, it may add unnecessary locking for 2. but I think that's
worth it to ensure that we are always on the safe side of thing. This idea is
implemented in LockNotPinnedObjectById().

A few remarks:

- there is one place where the relation is not visible (even if
CommandCounterIncrement() is used). That's in TypeCreate(), because the new 
relation Oid is _not_ added to pg_class yet.
Indeed, in heap_create_with_catalog(), AddNewRelationType() is called before
AddNewRelationTuple()). I put a comment in this part of the code explaining why
it's not necessary to call LockRelationOid() here.

- some namespace related stuff is removed from 
"test_oat_hooks/expected/alter_table.out".
That's due to the logic in cachedNamespacePath() and the fact that the same
namespace related stuff is added prior in alter_table.out.

- the patch touches 37 .c files, but that's mainly due to the fact that
LockNotPinnedObjectById() has to be called in a lot of places.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From d6c91b19a585d0a6f5c0abacb9c59cc5acd795f7 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Fri, 29 Mar 2024 15:43:26 +
Subject: [PATCH v12] Avoid orphaned objects dependencies

It's currently possible to create orphaned objects dependencies, for example:

Scenario 1:

session 1: begin; drop schema schem;
session 2: create a function in the schema schem
session 1: commit;

With the above, the function created in session 2 would be linked to a non
existing schema.

Scenario 2:

session 1: begin; create a function in the schema schem
session 2: drop schema schem;
session 1: commit;

With the above, the function created in session 1 would be linked to a non
existing schema.

To avoid those scenarios, a new lock (that conflicts with a lock taken by DROP)
has been put in place before the dependencies are being recorded. With this in
place, the drop schema in scenario 2 would be locked.

Also, after the new lock attempt, the patch checks that the object still exists:
with this in place session 2 in scenario 1 would be locked and would report an
error once session 1 committs (that would not be the case should session 1 abort
the transaction).

If the object is dropped before the new lock attempt is triggered then the patch
would also report an error (but with less details).

The patch takes into account any type of objects except the ones that are pinned
(they are not droppable because the system requires it).

A special case is done for objects that belong to the RelationRelationId class.
For those, we should be in one of the two following cases that would already
prevent the relation to be dropped:

1. The relation is already locked (could be an existing relation or a relation
that we are creating).

2. The relation is protected indirectly (i.e an index protected by a lock on
its table, a table protected by a lock on a function that depends the table...)

To avoid any risks for the RelationRelationId class case, we acquire a lock if
there is none. That may add unnecessary lock for 2. but that's worth it.

The patch adds a few tests for some dependency cases (that would currently produce
orphaned objects):

- schema and function (as the above scenarios)
- alter a dependency (function and schema)
- function and arg type
- function and return type
- function and function
- domain and domain
- table and type
- server and foreign data wrapper
---
 contrib/test_decoding/expected/twophase.out   |   3 +-
 src/backend/catalog/aclchk.c  |   1 +
 src/backend/catalog/dependency.c  | 125 -
 src/backend/catalog/heap.c|   7 +
 src/backend/catalog/index.c   |  26 
 src/backend/catalog/objectaddress.c   |  57 
 src/backend/catalog/pg_aggregate.c|   9 ++
 src/backend/catalog/pg_attrdef.c  |   1 +
 src/backend/catalog/pg_cast.c |   5 +
 src/backend/catalog/pg_collation.c|   1 +
 src/backend/catalog/pg_constraint.c   |  26 
 src/backend/catalog/pg_conversion.c   |   2 +
 src/backend/catalog/pg_depend.c   |  36 -
 src/backend/catalog/pg_operator.c |  19 +++
 src/backend/catalog/pg_proc.c |  17 ++-
 src/backend/catalog/pg_publication.c  |   7 +
 src/backend/catalog/pg_range.c|   6 +
 src/backend/catalog/pg_type.c |  39 ++
 src/backend/catalo

RE: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE

[Masahiro.Ikeda]

Hi,

Thanks for working it! I'm interested in this feature, so I'd like to 
participate
in the patch review. Though I've just started looking at the patch, I have two
comments about the v6 patch. (And I want to confirm the thread active.)


1) Unify the print format of leader and worker

In show_tidbitmap_info(), the number of exact/loosy blocks of the leader and
workers are printed. I think the printed format should be same. Currently, the
leader does not print the blocks of exact/lossy with a value of 0, but the 
workers
could even if it is 0.

IMHO, it's better to print both exact/lossy blocks if at least one of the 
numbers of
exact/lossy blocks is greater than 0. After all, the print logic is redundant 
for leader
and workers, but I thought it would be better to make it a common function.


2) Move es->workers_state check

In show_tidbitmap_info(), ExplainOpenWorker() and ExplainCloseWorker() are 
called
after checking es->worker_state is not NULL. However, es->workers_state seem to 
be
able to be NULL only for the Gather node (I see ExplainPrintPlan()). Also, 
reading the
comments, there is a description that each worker information needs to be 
hidden when
printing the plan.

Even if es->workers_state becomes NULL in BitmapHeapScan node in the future, I 
think
that workers' information(Heap Blocks) should not be printed. Therefore, I think
es->workers_state check should be move to the place of "if (planstate->pstate 
!= NULL)"
like ExplainNode(), doesn't it?

IIUC, we need to correct show_sort_info() and so on too…

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

pgindent exit status if a file encounters an error

[Ashutosh Bapat]

Hi All,

If pgindent encounters an error while applying pg_bsd_indent on a file, it
reports an error to stderr but does not exit with non-zero status.

$ src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2412: Stuff
missing from end of file

$ echo $?
0

A zero status usually indicates success [1]. In that sense pgindent
shouldn't be returning 0 in this case. It has not been able to process
file/s successfully. Not returning non-zero exit status in such cases means
we can not rely on `set -e` or `git rebase` s automatic detection of
command failures. I propose to add non-zero exit status in the above case.

In the attached patch I have used exit code 3 for file processing errors.
The program exits only after reporting all such errors instead of exiting
on the first instance. That way we get to know all the errors upfront. But
I am ok if we want to exit on the first instance. That might simplify its
interaction with other exit codes.

With this change, if I run pgident in `git rebase` it stops after those
errors automatically like below
```
Executing: src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2424: Stuff
missing from end of file

Failure in ./src/backend/optimizer/util/appendinfo.c: Error@1028: Stuff
missing from end of file

warning: execution failed: src/tools/pgindent/pgindent .
You can fix the problem, and then run

  git rebase --continue
```

I looked at pgindent README but it doesn't mention anything about exit
codes. So I believe this change is acceptable as per documentation.

With --check option pgindent reports a non-zero exit code instead of making
changes. So I feel the above change should happen at least if --check is
provided. But certainly better if we do it even without --check.

In case --check is specified and both the following happen a. pg_bsd_indent
exits with non-zero status while processing some file and b. changes are
produced while processing some other file, the program will exit with
status 2. It may be argued that instead it should exit with code 3. I am
open to both.

[1] https://en.wikipedia.org/wiki/Exit_status
-- 
Best Wishes,
Ashutosh Bapat
From f43a90ead02637896e375144b015b971b5e86fce Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Wed, 26 Jun 2024 15:46:53 +0530
Subject: [PATCH 1/2] pgindent exit status on error

When pg_bsd_indent exits with a non-zero status or reports an error, make
pgindent exit with non-zero status 3. The program does not exit on
the first instance of error. Instead it continues to process remaining
files as long as some other exit condition is encountered. In that case
exit status corresponding to that condition is reported.

This helps when pgindent is run in shell which interpret non-zero exit
status as failure and allow configuring actions on failure.

Author: Ashutosh Bapat
---
 src/tools/pgindent/pgindent | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 48d83bc434..7dbd3a67c2 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -409,6 +409,7 @@ foreach my $source_filename (@files)
 	if ($source eq "")
 	{
 		print STDERR "Failure in $source_filename: " . $error_message . "\n";
+		$status = 3;
 		next;
 	}
 
-- 
2.34.1

Re: remaining sql/json patches

[Alexander Lakhin]

Hello,

I'm not sure I've chosen the most appropriate thread for reporting the
issue, but maybe you would like to look at code comments related to
SQL/JSON constructors:

 * Transform JSON_ARRAY() constructor.
 *
 * JSON_ARRAY() is transformed into json[b]_build_array[_ext]() call
 * depending on the output JSON format. The first argument of
 * json[b]_build_array_ext() is absent_on_null.


 * Transform JSON_OBJECT() constructor.
 *
 * JSON_OBJECT() is transformed into json[b]_build_object[_ext]() call
 * depending on the output JSON format. The first two arguments of
 * json[b]_build_object_ext() are absent_on_null and check_unique.

But the referenced functions were removed at [1]; Nikita Glukhov wrote:

I have removed json[b]_build_object_ext() and json[b]_build_array_ext().


(That thread seems too old for the current discussion.)

Also, a comment above transformJsonObjectAgg() references
json[b]_objectagg[_unique][_strict](key, value), but I could find
json_objectagg() only.

[1] 
https://www.postgresql.org/message-id/be40362b-7821-7422-d33f-fbf1c61bb3e3%40postgrespro.ru

Best regards,
Alexander

Re: pg_createsubscriber: drop pre-existing subscriptions from the converted node

[Amit Kapila]

On Fri, Jun 21, 2024 at 4:51 PM Hayato Kuroda (Fujitsu)
 wrote:
>
> This is a follow-up thread for pg_createsubscriber [1]. I started a new thread
> since there is no activity around here.
>
> ## Problem
>
> Assuming that there is a cascading replication like below:
>
> node A --(logical replication)--> node B --(streaming replication)--> node C
>
> In this case, subscriptions exist even on node C, but it does not try to 
> connect
> to node A because the logical replication launcher/worker won't be launched.
> After the conversion, node C becomes a subscriber for node B, and the 
> subscription
> toward node A remains. Therefore, another worker that tries to connect to 
> node A
> will be launched, raising an ERROR [2]. This failure may occur even during the
> conversion.
>
> ## Solution
>
> The easiest solution is to drop pre-existing subscriptions from the converted 
> node.
> To avoid establishing connections during the conversion, slot_name is set to 
> NONE
> on the primary first, then drop on the standby. The setting will be restored 
> on the
> primary node.
>

It seems disabling subscriptions on the primary can make the primary
stop functioning for some duration of time. I feel we need some
solution where after converting to subscriber, we disable and drop
pre-existing subscriptions. One idea could be that we use the list of
new subscriptions created by the tool such that any subscription not
existing in that list will be dropped.

Shouldn't this be an open item for PG17?

-- 
With Regards,
Amit Kapila.

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

[Aleksander Alekseev]

Hi Noah,

> This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> 32-bit integers" and left some expressions coercing SLRU page numbers to int.
> Two sources:
>
>  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
>  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 
> | less -p '.int. from .int64. may alter its value'
>
> (Not every match needs to change.)

I examined the new warnings introduced by 4ed8f09. Most of them seem
to be harmless, for instance:

```
slru.c:657:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  657 | int rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

```
slru.c: In function ‘SlruReportIOError’:
slru.c:962:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  962 | int rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

Interestingly the patch decreased the overall number of warnings.

I prepared the patch for clog.c. The rest of the warnings don't strike
me as something we should immediately act on unless we have a bug
report. Or perhaps there is a particular warning that worries you?

> > @@ -127,7 +127,15 @@ typedef struct SlruCtlData
> >* the behavior of this callback has no functional implications.)  Use
> >* SlruPagePrecedesUnitTests() in SLRUs meeting its criteria.
> >*/
> > - bool(*PagePrecedes) (int, int);
> > + bool(*PagePrecedes) (int64, int64);
> > +
> > + /*
> > +  * If true, use long segment filenames formed from lower 48 bits of 
> > the
> > +  * segment number, e.g. pg_xact/1234. Otherwise, use short
> > +  * filenames formed from lower 16 bits of the segment number e.g.
> > +  * pg_xact/1234.
> > +  */
> > + boollong_segment_names;
>
> SlruFileName() makes 15-character (60-bit) file names.  Where does the 48-bit
> limit arise?  How does the SlruFileName() comment about a 24-bit limit for
> short names relate this comment's 16-bit limit?

Yes, this comment is wrong. Here is a fix.

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

-- 
Best regards,
Aleksander Alekseev


v1-0002-Use-int64-for-page-numbers-in-clog.c.patch
Description: Binary data


v1-0001-Fix-the-comment-for-SlruCtlData.long_segment_name.patch
Description: Binary data

Re: pgindent exit status if a file encounters an error

[Andrew Dunstan]

On 2024-06-26 We 6:37 AM, Ashutosh Bapat wrote:

Hi All,

If pgindent encounters an error while applying pg_bsd_indent on a 
file, it reports an error to stderr but does not exit with non-zero 
status.


$ src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2412: Stuff 
missing from end of file


$ echo $?
0

A zero status usually indicates success [1]. In that sense pgindent 
shouldn't be returning 0 in this case. It has not been able to process 
file/s successfully. Not returning non-zero exit status in such cases 
means we can not rely on `set -e` or `git rebase` s automatic 
detection of command failures. I propose to add non-zero exit status 
in the above case.


In the attached patch I have used exit code 3 for file processing 
errors. The program exits only after reporting all such errors instead 
of exiting on the first instance. That way we get to know all the 
errors upfront. But I am ok if we want to exit on the first instance. 
That might simplify its interaction with other exit codes.


With this change, if I run pgident in `git rebase` it stops after 
those errors automatically like below

```
Executing: src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2424: Stuff 
missing from end of file


Failure in ./src/backend/optimizer/util/appendinfo.c: Error@1028: 
Stuff missing from end of file


warning: execution failed: src/tools/pgindent/pgindent .
You can fix the problem, and then run

  git rebase --continue
```

I looked at pgindent README but it doesn't mention anything about exit 
codes. So I believe this change is acceptable as per documentation.


With --check option pgindent reports a non-zero exit code instead of 
making changes. So I feel the above change should happen at least if 
--check is provided. But certainly better if we do it even without 
--check.


In case --check is specified and both the following happen a. 
pg_bsd_indent exits with non-zero status while processing some file 
and b. changes are produced while processing some other file, the 
program will exit with status 2. It may be argued that instead it 
should exit with code 3. I am open to both.



Yeah, I think this is reasonable but we should adjust the status setting 
a few lines lower to



   $status ||= 2;


cheers


andrew


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

Re: long-standing data loss bug in initial sync of logical replication

[Tomas Vondra]

On 6/25/24 07:04, Amit Kapila wrote:
> On Mon, Jun 24, 2024 at 8:06 PM Tomas Vondra
>  wrote:
>>
>> On 6/24/24 12:54, Amit Kapila wrote:
>>> ...

>> I'm not sure there are any cases where using SRE instead of AE would 
>> cause
>> problems for logical decoding, but it seems very hard to prove. I'd be 
>> very
>> surprised if just using SRE would not lead to corrupted cache contents 
>> in some
>> situations. The cases where a lower lock level is ok are ones where we 
>> just
>> don't care that the cache is coherent in that moment.

> Are you saying it might break cases that are not corrupted now? How
> could obtaining a stronger lock have such effect?

 No, I mean that I don't know if using SRE instead of AE would have negative
 consequences for logical decoding. I.e. whether, from a logical decoding 
 POV,
 it'd suffice to increase the lock level to just SRE instead of AE.

 Since I don't see how it'd be correct otherwise, it's kind of a moot 
 question.

>>>
>>> We lost track of this thread and the bug is still open. IIUC, the
>>> conclusion is to use SRE in OpenTableList() to fix the reported issue.
>>> Andres, Tomas, please let me know if my understanding is wrong,
>>> otherwise, let's proceed and fix this issue.
>>>
>>
>> It's in the commitfest [https://commitfest.postgresql.org/48/4766/] so I
>> don't think we 'lost track' of it, but it's true we haven't done much
>> progress recently.
>>
> 
> Okay, thanks for pointing to the CF entry. Would you like to take care
> of this? Are you seeing anything more than the simple fix to use SRE
> in OpenTableList()?
> 

I did not find a simpler fix than adding the SRE, and I think pretty
much any other fix is guaranteed to be more complex. I don't remember
all the details without relearning all the details, but IIRC the main
challenge for me was to convince myself it's a sufficient and reliable
fix (and not working simply by chance).

I won't have time to look into this anytime soon, so feel free to take
care of this and push the fix.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Buildfarm animal caiman showing a plperl test issue with newer Perl versions

[Andrew Dunstan]

On 2024-06-24 Mo 6:46 AM, Andrew Dunstan wrote:


On 2024-06-24 Mo 12:00 AM, Alexander Lakhin wrote:

Hello hackers,

As recent caiman failures ([1], [2], ...) show, plperl.sql is 
incompatible

with Perl 5.40. (The last successful test runs took place when cayman
had Perl 5.38.2 installed: [3].)

FWIW, I've found an already-existing fix for the issue [4] and a note
describing the change for Perl 5.39.10 [5].

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-06-24%2001%3A34%3A23
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-06-24%2000%3A15%3A16
[3] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-05-02%2021%3A57%3A17
[4] 
https://git.alpinelinux.org/aports/tree/community/postgresql14/fix-test-plperl-5.8-pragma.patch?id=28aeb872811f59a7f646aa29ed7c9dc30e698e65
[5] 
https://metacpan.org/release/PEVANS/perl-5.39.10/changes#Selected-Bug-Fixes





It's a very odd bug. I guess we should just backpatch the removal of 
that redundant version check in plc_perlboot.pl, probably all the way 
down to 9.2 since godwit builds and tests with plperl that far back, 
and some day in the not too distant future it will upgrade to perl 5.40.






Done.


cheers


andrew

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

Re: Backporting BackgroundPsql

[Robert Haas]

On Wed, Jun 26, 2024 at 3:34 AM Heikki Linnakangas  wrote:
> I haven't looked closely at the new PgFFI stuff but +1 on that in
> general, and it makes sense to backport that once it lands on master. In
> the meanwhile, I think we should backport BackgroundPsql as it is, to
> make it possible to backport tests using it right now, even if it is
> short-lived.

+1. The fact that PgFFI may be coming isn't a reason to not back-patch
this. The risk of back-patching testing infrastructure is also very
low as compared with code; in fact, there's a lot of risk from NOT
back-patching popular testing infrastructure.

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

Re: ON ERROR in json_query and the like

[Amit Langote]

Hi,

On Sat, Jun 22, 2024 at 5:43 PM Amit Langote  wrote:
> On Fri, Jun 21, 2024 at 8:00 PM Markus Winand  wrote:
> > So updating the three options:
> > > 1. Disallow anything but jsonb for context_item (the patch I posted 
> > > yesterday)
> >
> > * Non-conforming
> > * patch available
> >
> > > 2. Continue allowing context_item to be non-json character or utf-8
> > > encoded bytea strings, but document that any parsing errors do not
> > > respect the ON ERROR clause.
> >
> > * Conforming by choosing IA050 to implement GR4: raise errors independent 
> > of the ON ERROR clause.
> > * currently committed.
> >
> > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> > > respect ON ERROR (no patch written yet).
> >
> > * Conforming by choosing IA050 not to implement GR4: Parsing happens later, 
> > considering the ON ERROR clause.
> > * no patch available, not trivial
> >
> > I guess I’m the only one in favour of 3 ;) My remaining arguments are that 
> > Oracle and Db2 (LUW) do it that way and also that it is IMHO what users 
> > would expect. However, as 2 is also conforming (how could I miss that?), 
> > proper documentation is a very tempting option.
>
> So, we should go with 2 for v17, because while 3 may be very
> appealing, there's a risk that it might not get done in the time
> remaining for v17.
>
> I'll post the documentation patch on Monday.

Here's that patch, which adds this note after table 9.16.3. SQL/JSON
Query Functions:

+  
+   
+The context_item expression is converted to
+jsonb by an implicit cast if the expression is not already of
+type jsonb. Note, however, that any parsing errors that occur
+during that conversion are thrown unconditionally, that is, are not
+handled according to the (specified or implicit) ON
ERROR
+clause.
+   
+  

Peter, sorry about the last-minute ask, but do you have any
thoughts/advice on conformance as discussed above?

--
Thanks, Amit Langote


v1-0001-SQL-JSON-Document-behavior-when-context_item-is-n.patch
Description: Binary data

Re: Pgoutput not capturing the generated columns

[Shubham Khanna]

On Mon, Jun 24, 2024 at 8:21 AM Peter Smith  wrote:
>
> Hi, here are some patch v9-0001 comments.
>
> I saw Kuroda-san has already posted comments for this patch so there
> may be some duplication here.
>
> ==
> GENERAL
>
> 1.
> The later patches 0002 etc are checking to support only STORED
> gencols. But, doesn't that restriction belong in this patch 0001 so
> VIRTUAL columns are not decoded etc in the first place... (??)
>
> ~~~
>
> 2.
> The "Generated Columns" docs mentioned in my previous review comment
> [2] should be modified by this 0001 patch.
>
> ~~~
>
> 3.
> I think the "Message Format" page mentioned in my previous review
> comment [3] should be modified by this 0001 patch.
>
> ==
> Commit message
>
> 4.
> The patch name is still broken as previously mentioned [1, #1]
>
> ==
> doc/src/sgml/protocol.sgml
>
> 5.
> Should this docs be referring to STORED generated columns, instead of
> just generated columns?
>
> ==
> doc/src/sgml/ref/create_subscription.sgml
>
> 6.
> Should this be docs referring to STORED generated columns, instead of
> just generated columns?
>
> ==
> src/bin/pg_dump/pg_dump.c
>
> getSubscriptions:
> NITPICK - tabs
> NITPICK - patch removed a blank line it should not be touching
> NITPICK = patch altered indents it should not be touching
> NITPICK - a missing blank line that was previously present
>
> 7.
> + else
> + appendPQExpBufferStr(query,
> + " false AS subincludegencols,\n");
>
> There is an unwanted comma here.
>
> ~
>
> dumpSubscription
> NITPICK - patch altered indents it should not be touching
>
> ==
> src/bin/pg_dump/pg_dump.h
>
> NITPICK - unnecessary blank line
>
> ==
> src/bin/psql/describe.c
>
> describeSubscriptions
> NITPICK - bad indentation
>
> 8.
> In my previous review [1, #4b] I suggested this new column should be
> in a different order (e.g. adjacent to the other ones ahead of
> 'Conninfo'), but this is not yet addressed.
>
> ==
> src/test/subscription/t/011_generated.pl
>
> NITPICK - missing space in comment
> NITPICK - misleading "because" wording in the comment
>
> ==
>
> 99.
> See also my attached nitpicks diff, for cosmetic issues. Please apply
> whatever you agree with.
>
> ==
> [1] My v8-0001 review -
> https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com
> [2] 
> https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com
> [3] 
> https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i2g%40mail.gmail.com

All the comments are handled.

I have attached the updated patch v11 here in [1]. See [1] for the
changes added.

[1] 
https://www.postgresql.org/message-id/CAHv8RjJpS_XDkR6OrsmMZtCBZNxeYoCdENhC0%3Dbe0rLmNvhiQw%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.

Re: New standby_slot_names GUC in PG 17

[Bertrand Drouvot]

Hi,

On Wed, Jun 26, 2024 at 09:15:48AM +, Zhijie Hou (Fujitsu) wrote:
> Renamed these to the names suggested by Amit.
> 
> Attach the v2 patch set which addressed above and removed
> the changes in release-17.sgml according to the comment from Amit.
> 

Thanks! LGTM.

Regards,

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

Re: pgsql: Add more SQL/JSON constructor functions

[Amit Langote]

On Fri, Jun 21, 2024 at 10:48 PM Amit Langote  wrote:
> On Fri, Jun 21, 2024 at 4:05 PM jian he  wrote:
> > hi.
> > i am a little confused.
> >
> > here[1] tom says:
> > > Yeah, I too think this is a cast, and truncation is the spec-defined
> > > behavior for casting to varchar with a specific length limit.  I see
> > > little reason that this should work differently from
> > >
> > > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> > >  json_serialize
> > > 
> > >  {"a":
> > > (1 row)
> >
> > if i understand it correctly, and my english interpretation is fine.
> > i think tom means something like:
> >
> > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
> > json_serialize('{"a":1, "a":2}' returning varchar(5));
> >
> > should return true.
> > the master will return true, but apply your patch, the above query
> > will yield an error.
>
> The RETURNING variant giving an error is what the standard asks us to
> do apparently.  I read Tom's last message on this thread as agreeing
> to that, even though hesitantly.  He can correct me if I got that
> wrong.
>
> > your patch will make domain and char(n) behavior inconsistent.
> > create domain char2 as char(2);
> > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
> >
> >
> > another example:
> > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > default '"aaa"'::jsonb ON ERROR);
> > same value (jsonb "aaa") error on error will yield error,
> > but `default expression on error` can coerce the value to char(2),
> > which looks a little bit inconsistent, I think.
>
> Interesting examples, thanks for sharing.
>
> Attached updated version should take into account that typmod may be
> hiding under domains.  Please test.

I'd like to push this one tomorrow, barring objections.

I could use some advice on backpatching.  As I mentioned upthread,
this changes the behavior for JSON_OBJECT(), JSON_ARRAY(),
JSON_ARRAYAGG(), JSON_OBJECTAGG() too, which were added in v16.
Should this change be backpatched?  In general, what's our stance on
changes that cater to improving standard compliance, but are not
necessarily bugs.

-- 
Thanks, Amit Langote

Re: New standby_slot_names GUC in PG 17

[Amit Kapila]

On Wed, Jun 26, 2024 at 6:00 PM Bertrand Drouvot
 wrote:
>
> On Wed, Jun 26, 2024 at 09:15:48AM +, Zhijie Hou (Fujitsu) wrote:
> > Renamed these to the names suggested by Amit.
> >
> > Attach the v2 patch set which addressed above and removed
> > the changes in release-17.sgml according to the comment from Amit.
> >
>
> Thanks! LGTM.
>

As per my reading of this thread, we have an agreement on changing the
GUC name standby_slot_names to synchronized_standby_slots. I'll wait
for a day and push the change unless someone thinks otherwise.

-- 
With Regards,
Amit Kapila.

doc: modify the comment in function libpqrcv_check_conninfo()

[ikedarintarof]

Hi,

The function 'libpqrcv_check_conninfo()' returns 'void', but the comment 
above says that the function returns true or false.

I've attached a patch to modify the comment.

Regard,
Rintaro Ikedadiff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..851bf44fd3 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -309,8 +309,8 @@ bad_connection:
  * local filesystem access to be attempted.
  *
  * If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If it can, it will do nothing if this connection string
+ * specifies a password and raise an error otherwise.
  */
 static void
 libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)

Re: pgindent exit status if a file encounters an error

[Ashutosh Bapat]

Hi Andrew,
Thanks for the quick review.

On Wed, Jun 26, 2024 at 4:53 PM Andrew Dunstan  wrote:

>
> With --check option pgindent reports a non-zero exit code instead of
> making changes. So I feel the above change should happen at least if
> --check is provided. But certainly better if we do it even without --check.
>
> In case --check is specified and both the following happen a.
> pg_bsd_indent exits with non-zero status while processing some file and b.
> changes are produced while processing some other file, the program will
> exit with status 2. It may be argued that instead it should exit with code
> 3. I am open to both.
>
>
> Yeah, I think this is reasonable but we should adjust the status setting a
> few lines lower to
>
>
>$status ||= 2;
>

So you are suggesting that status 3 is preferred over status 2 when both
are applicable. I am fine with that.

Here's what the behaviour looks like: (notice echo $? after running
pgindent)

1. Running without --check, if pgindent encounters file processing errors,
exit code is 3.
2. Running with --check, exit code depends upon whether pgindent encounters
a file with processing error first or a file that undergoes a change.
   a. If it encounters a file that would undergo a change first, exit
status is 2
   b. If it encounters a file with processing error first, exit status is 3
3. If --check is specified and no file undergoes a change, but there are
file processing errors, it will exit with code 3.

The variation in the second case based on the order of files processed
looks fine to me. What do you say?

The usage help mentions exit code 2 specifically while describing --check
option but it doesn't mention exit code 1. Neither does the README. So I
don't think we need to document exit code 3 anywhere. Please let me know if
you think otherwise.

-- 
Best Wishes,
Ashutosh Bapat
From 7a0f65a2367885eea0c9b7a4257e6faf758f58eb Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Wed, 26 Jun 2024 15:46:53 +0530
Subject: [PATCH 1/2] pgindent exit status on error

When pg_bsd_indent exits with a non-zero status or reports an error,
make pgindent exit with non-zero status 3. The program does not exit on
the first instance of the error. Instead it continues to process
remaining files as long as some other exit condition is encountered, in
which case exit code 3 is reported.

This helps to detect errors automatically when pgindent is run in shells
which interpret non-zero exit status as failure.

Author: Ashutosh Bapat
Reviewed by: Andrew Dunstan
Discussion: https://www.postgresql.org/message-id/caexhw5sprsifeldp-u1fa5ba7ys2f0gvljmkoobopkadjwq...@mail.gmail.com
---
 src/tools/pgindent/pgindent | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 48d83bc434..c9a8fd6561 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -409,6 +409,7 @@ foreach my $source_filename (@files)
 	if ($source eq "")
 	{
 		print STDERR "Failure in $source_filename: " . $error_message . "\n";
+		$status = 3;
 		next;
 	}
 
@@ -429,7 +430,7 @@ foreach my $source_filename (@files)
 
 			if ($check)
 			{
-$status = 2;
+$status ||= 2;
 last unless $diff;
 			}
 		}
-- 
2.34.1

Re: Should we document how column DEFAULT expressions work?

[Alvaro Herrera]

On 2024-Jun-25, David G. Johnston wrote:

> On Tue, Jun 25, 2024 at 9:50 PM David Rowley  wrote:

> > FWIW, I disagree that we need to write anything about that in this
> > part of the documentation.  I think any argument for doing this could
> > equally be applied to something like re-iterating what the operator
> > precedence rules for arithmetic are, and I don't think that should be
> > mentioned.
> 
> I disagree on this equivalence.  The time literals are clear deviations
> from expected behavior.  Knowing operator precedence rules, they apply
> everywhere equally.  And we should document the deviations directly where
> they happen.  Even if it's just a short link back to the source that
> describes the deviation.  I'm fine with something less verbose pointing
> only to the data types page, but not with nothing.

I agree that it'd be good to have _something_ -- the other stance seems
super unhelpful.  "We're not going to spend two lines to explain some
funny rules that determine surprising behavior here, because we assume
you have read all of our other 3000 pages of almost impenetrably dense
documentation" is not great from a user's point of view.  The behavior
of 'now' in DEFAULT clauses is something that has been asked about for
decades.

Arithmetic precedence is a terrible straw man argument.  Let's put that
aside.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)

Re: psql (PostgreSQL) 17beta2 (Debian 17~beta2-1.pgdg+~20240625.1534.g23c5a0e) Failed to retrieve data from the server..

[Bruce Momjian]

On Wed, Jun 26, 2024 at 12:06:13AM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Tue, Jun 25, 2024 at 05:52:47PM -0700, David G. Johnston wrote:
> >> We seem to be missing a release note item for the catalog breakage done in
> >> f696c0c
> >> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f696c0cd5f
> 
> > It seemed too internal to mention in the release notes --- more of an
> > infrastructure change, but I can add it if I was wrong about this.
> 
> As this breakage demonstrates, that change is quite
> application-visible.  It needs an entry under incompatibilities.

Okay, will do today.

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

  Only you can decide what is important to you.

Re: [PATCH] Add ACL (Access Control List) acronym

[Nathan Bossart]

On Tue, Jun 25, 2024 at 05:59:01PM -0700, David G. Johnston wrote:
> Though there was no comment on the fact we should be linking to:
> 
> https://en.wikipedia.org/wiki/Access-control_list
> 
> not:
> 
> https://en.wikipedia.org/wiki/Access_Control_List
> 
> to avoid the dis-ambiguation redirect.

+1

-- 
nathan

Re: doc: modify the comment in function libpqrcv_check_conninfo()

[Jelte Fennema-Nio]

On Wed, 26 Jun 2024 at 14:53, ikedarintarof
 wrote:
> The function 'libpqrcv_check_conninfo()' returns 'void', but the comment
> above says that the function returns true or false.
> I've attached a patch to modify the comment.

Agreed that the current comment is wrong, but the new comment should
mention the must_use_password argument. Because only if
must_use_password is true, will it throw an error.

Re: Wrong security context for deferred triggers?

[David G. Johnston]

On Wed, Jun 26, 2024 at 2:02 AM Laurenz Albe 
wrote:

>
> I think that we should have some consensus about the following before
> we discuss syntax:
>
> - Does anybody depend on the current behavior and would be hurt if
>   my current patch went in as it is?
>
> - Is this worth changing at all or had we better document the current
>   behavior and leave it as it is?
>
> Concerning the latter, I am hoping for a detailed description of our
> customer's use case some time soon.
>
>
We have a few choices then:
1. Status quo + documentation backpatch
2. Change v18 narrowly + documentation backpatch
3. Backpatch narrowly (one infers the new behavior after reading the
existing documentation)
4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow
change to fix the POLA violation

I've been presenting option 4.

Pondering further, I see now that having the owner-execution mode be the
only way to avoid the POLA violation in deferred triggers isn't great since
many triggers benefit from the implied security of being able to run in the
invoker's execution context - especially if the trigger doesn't do anything
that PUBLIC cannot already do.

So, I'm on board with option 2 at this point.

David J.

Re: improve predefined roles documentation

[Robert Haas]

On Tue, Jun 25, 2024 at 4:19 PM Nathan Bossart  wrote:
> Yeah, my options were to either separate the roles or to weaken the
> ordering, and I guess I felt like the weaker ordering was slightly less
> bad.  The extra context in some of the groups seemed worth keeping, and
> this probably isn't the only page of our docs that might require ctrl+f.
> But I'll yield to the majority opinion here.

I'm not objecting. I'm just asking.

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

Re: pgsql: Add more SQL/JSON constructor functions

[jian he]

On Wed, Jun 26, 2024 at 8:39 PM Amit Langote  wrote:
>
> >
> > The RETURNING variant giving an error is what the standard asks us to
> > do apparently.  I read Tom's last message on this thread as agreeing
> > to that, even though hesitantly.  He can correct me if I got that
> > wrong.
> >
> > > your patch will make domain and char(n) behavior inconsistent.
> > > create domain char2 as char(2);
> > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
> > >
> > >
> > > another example:
> > > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > > default '"aaa"'::jsonb ON ERROR);
> > > same value (jsonb "aaa") error on error will yield error,
> > > but `default expression on error` can coerce the value to char(2),
> > > which looks a little bit inconsistent, I think.
> >
> > Interesting examples, thanks for sharing.
> >
> > Attached updated version should take into account that typmod may be
> > hiding under domains.  Please test.
>

SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
'13' on error);
return
ERROR:  value too long for type character(2)
should return 13

I found out the source of the problem is in coerceJsonExprOutput
/*
* Use cast expression for domain types; we need CoerceToDomain here.
*/
if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
{
jsexpr->use_io_coercion = true;
return;
}

>
> I'd like to push this one tomorrow, barring objections.
>

Currently the latest patch available cannot be `git apply` cleanly.

@@ -464,3 +466,9 @@ SELECT JSON_QUERY(jsonb 'null', '$xyz' PASSING 1 AS xyz);
 SELECT JSON_EXISTS(jsonb '1', '$' DEFAULT 1 ON ERROR);
 SELECT JSON_VALUE(jsonb '1', '$' EMPTY ON ERROR);
 SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR);
+
+-- Test implicit coercion domain over fixed-legth type specified in RETURNING
+CREATE DOMAIN queryfuncs_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
+SELECT JSON_QUERY(jsonb '123', '$' RETURNING queryfuncs_char2 ERROR ON ERROR);
+SELECT JSON_VALUE(jsonb '123', '$' RETURNING queryfuncs_char2 ERROR ON ERROR);
+SELECT JSON_VALUE(jsonb '12', '$' RETURNING queryfuncs_char2 ERROR ON ERROR);

cannot found `SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR);`  in
https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/sql/sqljson_queryfuncs.sql

Re: [PATCH] Add ACL (Access Control List) acronym

[Joel Jacobson]

On Wed, Jun 26, 2024, at 02:59, David G. Johnston wrote:
> Though there was no comment on the fact we should be linking to:
>
> https://en.wikipedia.org/wiki/Access-control_list
>
> not:
>
> https://en.wikipedia.org/wiki/Access_Control_List
>
> to avoid the dis-ambiguation redirect.
>
> If we are making wikipedia our authority we might as well use their 
> standard for naming.

Good point.

Want me to fix that or will the committer handle that?

I found some more similar cases in acronyms.sgml.

-https://en.wikipedia.org/wiki/Pluggable_Authentication_Modules
+https://en.wikipedia.org/wiki/Pluggable_authentication_module
-https://en.wikipedia.org/wiki/Data_Manipulation_Language
+https://en.wikipedia.org/wiki/Data_manipulation_language
-https://en.wikipedia.org/wiki/OLTP
+https://en.wikipedia.org/wiki/Online_transaction_processing
-https://en.wikipedia.org/wiki/Data_Definition_Language
+https://en.wikipedia.org/wiki/Data_definition_language
-https://en.wikipedia.org/wiki/ORDBMS
+https://en.wikipedia.org/wiki/Object%E2%80%93relational_database
-https://en.wikipedia.org/wiki/GMT
+https://en.wikipedia.org/wiki/Greenwich_Mean_Time
-https://en.wikipedia.org/wiki/Relational_database_management_system
+https://en.wikipedia.org/wiki/Relational_database#RDBMS
-https://en.wikipedia.org/wiki/Olap
-https://en.wikipedia.org/wiki/Issn
+https://en.wikipedia.org/wiki/Online_analytical_processing
+https://en.wikipedia.org/wiki/ISSN
-https://en.wikipedia.org/wiki/System_V
+https://en.wikipedia.org/wiki/UNIX_System_V
-https://en.wikipedia.org/wiki/Visual_C++
+https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B
-https://en.wikipedia.org/wiki/SGML
+https://en.wikipedia.org/wiki/Standard_Generalized_Markup_Language
-https://en.wikipedia.org/wiki/Ascii
+https://en.wikipedia.org/wiki/ASCII
-https://en.wikipedia.org/wiki/Dbms
+https://en.wikipedia.org/wiki/Database#Database_management_system
-https://en.wikipedia.org/wiki/Git_(software)
+https://en.wikipedia.org/wiki/Git
-https://en.wikipedia.org/wiki/Utf8
+https://en.wikipedia.org/wiki/UTF-8
-https://en.wikipedia.org/wiki/Secure_Sockets_Layer
+https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0,_2.0,_and_3.0

Below is the script I used to find them,
which also reports some additional false positives:

```
#!/bin/bash

export LC_ALL=C
wget -q -O acronyms.html https://www.postgresql.org/docs/current/acronyms.html
urls=$(grep -o 'https://[^"]*' acronyms.html)
output_file="canonical_urls.txt"
> $output_file

extract_canonical() {
  local url=$1
  canonical=$(curl -s $url | sed -n 's/.*> $output_file
echo "+$canonical" >> $output_file
  fi
}

for url in $urls; do
  extract_canonical $url &
done

wait

cat $output_file
```

/Joel

Re: [PATCH] Add ACL (Access Control List) acronym

[David G. Johnston]

On Wed, Jun 26, 2024 at 7:52 AM Joel Jacobson  wrote:

> On Wed, Jun 26, 2024, at 02:59, David G. Johnston wrote:
> > Though there was no comment on the fact we should be linking to:
> >
> > https://en.wikipedia.org/wiki/Access-control_list
> >
> > not:
> >
> > https://en.wikipedia.org/wiki/Access_Control_List
> >
> > to avoid the dis-ambiguation redirect.
> >
> > If we are making wikipedia our authority we might as well use their
> > standard for naming.
>
> Good point.
>
> Want me to fix that or will the committer handle that?
>
> I found some more similar cases in acronyms.sgml.
>
> -https://en.wikipedia.org/wiki/Pluggable_Authentication_Modules
> +https://en.wikipedia.org/wiki/Pluggable_authentication_module
> -https://en.wikipedia.org/wiki/Data_Manipulation_Language
> +https://en.wikipedia.org/wiki/Data_manipulation_language
> -https://en.wikipedia.org/wiki/OLTP
> +https://en.wikipedia.org/wiki/Online_transaction_processing
> -https://en.wikipedia.org/wiki/Data_Definition_Language
> +https://en.wikipedia.org/wiki/Data_definition_language
> -https://en.wikipedia.org/wiki/ORDBMS
> +https://en.wikipedia.org/wiki/Object%E2%80%93relational_database
> -https://en.wikipedia.org/wiki/GMT
> 
> +https://en.wikipedia.org/wiki/Greenwich_Mean_Time
> -https://en.wikipedia.org/wiki/Relational_database_management_system
> +https://en.wikipedia.org/wiki/Relational_database#RDBMS
> -https://en.wikipedia.org/wiki/Olap
> -https://en.wikipedia.org/wiki/Issn
> +https://en.wikipedia.org/wiki/Online_analytical_processing
> +https://en.wikipedia.org/wiki/ISSN
> -https://en.wikipedia.org/wiki/System_V
> +https://en.wikipedia.org/wiki/UNIX_System_V
> -https://en.wikipedia.org/wiki/Visual_C++
> +https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B
> -https://en.wikipedia.org/wiki/SGML
> +https://en.wikipedia.org/wiki/Standard_Generalized_Markup_Language
> -https://en.wikipedia.org/wiki/Ascii
> 
> +https://en.wikipedia.org/wiki/ASCII
> -https://en.wikipedia.org/wiki/Dbms
> +https://en.wikipedia.org/wiki/Database#Database_management_system
> -https://en.wikipedia.org/wiki/Git_(software)
> 
> +https://en.wikipedia.org/wiki/Git
> -https://en.wikipedia.org/wiki/Utf8
> +https://en.wikipedia.org/wiki/UTF-8
> -https://en.wikipedia.org/wiki/Secure_Sockets_Layer
> +
> https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_1.0,_2.0,_and_3.0
>
> Below is the script I used to find them,
> which also reports some additional false positives:
>
>
Given this I'd be OK with committing as-is in the name of matching existing
project style.  Then bringing up this inconsistency as a separate concern
to be bulk fixed as part of implementing a new policy on what to check for
and conform to when establishing acronyms in our documentation.

Otherwise the author (you) should make the change here - the committer
wouldn't be expected to know to do that from the discussion.

David J.

Incremental Sort Cost Estimation Instability

[Andrei Lepikhov]

Hi,

While designing an improvement for the cost sort model, I discovered 
that the query plan can vary if we slightly change the query text 
without pushing semantic differences. See the example below:


CREATE TABLE test(x integer, y integer,z text);
INSERT INTO test (x,y) SELECT x, 1 FROM generate_series(1,100) AS x;
CREATE INDEX ON test(x);
CREATE INDEX ON test(y);
VACUUM ANALYZE;
SET max_parallel_workers_per_gather = 0;

First query:

EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM test t1, test t2
WHERE t1.x=t2.y AND t1.y=t2.x GROUP BY t1.x,t1.y;

And the second one - just reverse the left and right sides of 
expressions in the WHERE condition:


EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM test t1, test t2
WHERE t2.y=t1.x AND t2.x=t1.y GROUP BY t1.x,t1.y;

You can see two different plans here:

GroupAggregate  (cost=37824.89..37824.96 rows=1 width=16)
  Group Key: t1.y, t1.x
  ->  Incremental Sort  (cost=37824.89..37824.94 rows=2 width=8)
Sort Key: t1.y, t1.x
Presorted Key: t1.y
->  Merge Join  (cost=0.85..37824.88 rows=1 width=8)
  Merge Cond: (t1.y = t2.x)
  Join Filter: (t2.y = t1.x)
  ->  Index Scan using test_y_idx on test t1
  ->  Index Scan using test_x_idx on test t2

GroupAggregate  (cost=37824.89..37824.92 rows=1 width=16)
  Group Key: t1.x, t1.y
  ->  Sort  (cost=37824.89..37824.90 rows=1 width=8)
Sort Key: t1.x, t1.y
Sort Method: quicksort  Memory: 25kB
->  Merge Join  (cost=0.85..37824.88 rows=1 width=8)
  Merge Cond: (t1.y = t2.x)
  Join Filter: (t2.y = t1.x)
  ->  Index Scan using test_y_idx on test t1
  ->  Index Scan using test_x_idx on test t2

Don't mind for now that the best plan is to do IncrementalSort with 
presorted key t1.x. Just pay attention to the fact that the plan has 
altered without any valuable reason.
The cost_incremental_sort() routine causes such behaviour: it chooses 
the expression to estimate the number of groups from the first 
EquivalenceClass member that relies on the syntax.
I tried to invent a simple solution to fight this minor case. But the 
most clear and straightforward way here is to save a reference to the 
expression that triggered the PathKey creation inside the PathKey itself.

See the sketch of the patch in the attachment.
I'm not sure this instability is worth fixing this way, but the 
dependence of the optimisation outcome on the query text looks buggy.


--
regards, Andrei LepikhovFrom 8eb9998fe3306005d5067e53fef6a032515ebed5 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" 
Date: Wed, 26 Jun 2024 16:28:12 +0700
Subject: [PATCH] Add a reference to the expression which has triggered
 creation of this pathkey.

For the sake of consistency in number of groups estimation in incremental sort
we need to have an information about expression which caused the pathkey. It
doesn't guarantee correct estimation but at least estimation doesn't rely on
a SQL string.
---
 contrib/postgres_fdw/postgres_fdw.c   |  6 +++--
 src/backend/optimizer/path/costsize.c | 28 +++
 src/backend/optimizer/path/pathkeys.c | 33 ++-
 src/include/nodes/pathnodes.h |  2 ++
 src/include/optimizer/paths.h |  3 ++-
 5 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/contrib/postgres_fdw/postgres_fdw.c 
b/contrib/postgres_fdw/postgres_fdw.c
index 0bb9a5ae8f..da26d99fb1 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -976,6 +976,7 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, 
RelOptInfo *rel)
foreach(lc, useful_eclass_list)
{
EquivalenceClass *cur_ec = lfirst(lc);
+   EquivalenceMember *em;
PathKey*pathkey;
 
/* If redundant with what we did above, skip it. */
@@ -988,14 +989,15 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, 
RelOptInfo *rel)
continue;
 
/* If no pushable expression for this rel, skip it. */
-   if (find_em_for_rel(root, cur_ec, rel) == NULL)
+   if ((em = find_em_for_rel(root, cur_ec, rel)) == NULL)
continue;
 
/* Looks like we can generate a pathkey, so let's do it. */
pathkey = make_canonical_pathkey(root, cur_ec,

 linitial_oid(cur_ec->ec_opfamilies),

 BTLessStrategyNumber,
-   
 false);
+   
 false,
+   
 em->em_expr); /* TODO */
useful_pathkeys_list = lappend(useful_pathkeys_

Re: pgindent exit status if a file encounters an error

[Tom Lane]

Ashutosh Bapat  writes:
> The usage help mentions exit code 2 specifically while describing --check
> option but it doesn't mention exit code 1. Neither does the README. So I
> don't think we need to document exit code 3 anywhere. Please let me know if
> you think otherwise.

I think we should have at least a code comment summarizing the
possible exit codes, along the lines of

# Exit codes:
#   0 -- all OK
#   1 -- could not invoke pgindent, nothing done
#   2 -- --check mode and at least one file requires changes
#   3 -- pgindent failed on at least one file

regards, tom lane

Re: [PATCH] Add ACL (Access Control List) acronym

[Nathan Bossart]

On Wed, Jun 26, 2024 at 07:58:55AM -0700, David G. Johnston wrote:
> On Wed, Jun 26, 2024 at 7:52 AM Joel Jacobson  wrote:
>> Want me to fix that or will the committer handle that?
>>
>> I found some more similar cases in acronyms.sgml.
>
> Given this I'd be OK with committing as-is in the name of matching existing
> project style.  Then bringing up this inconsistency as a separate concern
> to be bulk fixed as part of implementing a new policy on what to check for
> and conform to when establishing acronyms in our documentation.
> 
> Otherwise the author (you) should make the change here - the committer
> wouldn't be expected to know to do that from the discussion.

If I was writing these patches, I'd create a separate 0001 patch to fix the
existing problems, then 0002 would be just the new stuff (without the
inconsistency).  But that's just what I'd do; there's no problem with doing
it the other way around.

-- 
nathan

Re: improve predefined roles documentation

[Nathan Bossart]

On Wed, Jun 26, 2024 at 10:40:10AM -0400, Robert Haas wrote:
> On Tue, Jun 25, 2024 at 4:19 PM Nathan Bossart  
> wrote:
>> Yeah, my options were to either separate the roles or to weaken the
>> ordering, and I guess I felt like the weaker ordering was slightly less
>> bad.  The extra context in some of the groups seemed worth keeping, and
>> this probably isn't the only page of our docs that might require ctrl+f.
>> But I'll yield to the majority opinion here.
> 
> I'm not objecting. I'm just asking.

Cool.  I'll plan on committing this latest version once v18devel hacking
begins.

-- 
nathan

Reg: Alternate way of hashing database role passwords

[M, Anbazhagan]

Hi Team,

Currently we are using SHA-256 default for password_encryption in our 
postgresql deployments. Is there any active work being done for adding 
additional hashing options like PBKDF2, HKDF, SCRYPT or Argon2 password hashing 
functions, either of which is only accepted as a algorithms that should be used 
for encrypting or hashing the password at storage as per the Organization's 
Cryptography Standard.

If it is not in current plan, is there a plan to include that in subsequent 
versions?

Thanks and Regards,
Anbazhagan M

Re: Wrong security context for deferred triggers?

[Laurenz Albe]

On Wed, 2024-06-26 at 07:38 -0700, David G. Johnston wrote:
> We have a few choices then:
> 1. Status quo + documentation backpatch
> 2. Change v18 narrowly + documentation backpatch
> 3. Backpatch narrowly (one infers the new behavior after reading the existing 
> documentation)
> 4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow change 
> to fix the POLA violation
> 
> I've been presenting option 4.
> 
> Pondering further, I see now that having the owner-execution mode be the only 
> way to avoid
> the POLA violation in deferred triggers isn't great since many triggers 
> benefit from the
> implied security of being able to run in the invoker's execution context - 
> especially if
> the trigger doesn't do anything that PUBLIC cannot already do.
> 
> So, I'm on board with option 2 at this point.

Nice.

I think we can safely rule out option 3.
Even if it is a bug, it is not one that has bothered anybody so far that a 
backpatch
is indicated.

Yours,
Laurenz Albe

Re: Reg: Alternate way of hashing database role passwords

[Tom Lane]

"M, Anbazhagan"  writes:
> Currently we are using SHA-256 default for password_encryption in our 
> postgresql deployments. Is there any active work being done for adding 
> additional hashing options like PBKDF2, HKDF, SCRYPT or Argon2 password 
> hashing functions, either of which is only accepted as a algorithms that 
> should be used for encrypting or hashing the password at storage as per the 
> Organization's Cryptography Standard.

> If it is not in current plan, is there a plan to include that in subsequent 
> versions?

It is not, and I doubt we have any interest in dramatically expanding
the set of allowed password hashes.  Adding SCRAM was enough work and
created a lot of client-v-server and cross-version incompatibility
already; nobody is in a hurry to repeat that.  Moreover, I know of
no reason to think that SHA-256 isn't perfectly adequate.

regards, tom lane

Re: libpq: Fix lots of discrepancies in PQtrace

[Nathan Bossart]

On Fri, Jun 21, 2024 at 04:01:55PM -0500, Nathan Bossart wrote:
> On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
>> 0001 is some cleanup after f4b54e1ed9
> 
> Oops.  I'll plan on committing this after the 17beta2 release freeze is
> lifted.

Committed 0001.

-- 
nathan

Re: libpq: Fix lots of discrepancies in PQtrace

[Alvaro Herrera]

On 2024-Jun-26, Nathan Bossart wrote:

> On Fri, Jun 21, 2024 at 04:01:55PM -0500, Nathan Bossart wrote:
> > On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
> >> 0001 is some cleanup after f4b54e1ed9
> > 
> > Oops.  I'll plan on committing this after the 17beta2 release freeze is
> > lifted.
> 
> Committed 0001.

Thanks, Nathan.  I'm holding myself responsible for the rest ... will
handle soon after the branch.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)

Re: [PATCH] Add ACL (Access Control List) acronym

[David G. Johnston]

On Wed, Jun 26, 2024 at 8:47 AM Nathan Bossart 
wrote:

> On Wed, Jun 26, 2024 at 07:58:55AM -0700, David G. Johnston wrote:
> > On Wed, Jun 26, 2024 at 7:52 AM Joel Jacobson  wrote:
> >> Want me to fix that or will the committer handle that?
> >>
> >> I found some more similar cases in acronyms.sgml.
> >
> > Given this I'd be OK with committing as-is in the name of matching
> existing
> > project style.  Then bringing up this inconsistency as a separate concern
> > to be bulk fixed as part of implementing a new policy on what to check
> for
> > and conform to when establishing acronyms in our documentation.
> >
> > Otherwise the author (you) should make the change here - the committer
> > wouldn't be expected to know to do that from the discussion.
>
> If I was writing these patches, I'd create a separate 0001 patch to fix the
> existing problems, then 0002 would be just the new stuff (without the
> inconsistency).  But that's just what I'd do; there's no problem with doing
> it the other way around.
>
>
Agreed, if Joel wants to write both.  But as the broader fix shouldn't
block adding a new acronym, it doesn't make sense to insist on this
approach.  Consistency makes sense though doing it the expected way would
be OK as well.  Either way, assuming the future patch materializes and gets
committed the end state is the same, and the path to it doesn't really
matter.

David J.

Re: Reg: Alternate way of hashing database role passwords

[Robert Haas]

On Wed, Jun 26, 2024 at 12:11 PM Tom Lane  wrote:
> It is not, and I doubt we have any interest in dramatically expanding
> the set of allowed password hashes.  Adding SCRAM was enough work and
> created a lot of client-v-server and cross-version incompatibility
> already; nobody is in a hurry to repeat that.  Moreover, I know of
> no reason to think that SHA-256 isn't perfectly adequate.

If history is any guide, every algorithm will eventually look too
weak. It seems inevitable that we're going to have to keep changing
algorithms as time passes. However, it seems like SCRAM is designed so
that different hash functions can be substituted into it, so what I'm
hoping is that we can keep SCRAM and just replace SCRAM-SHA-256 with
SCRAM-WHATEVER when SHA-256 starts to look too weak.

What I find a bit surprising about Anbazhagan's question is that he
asks about PBKDF2, which seems to be part of SCRAM already.[1] In
fact, I think all the things he lists are key derivation functions,
not hash functions. I'm far from a cryptography expert, but it seems
surprising to me that somebody would be concerned about the KDF rather
than the hash function. We know that people get concerned about code
that still uses MD5, for example, or SHA-1, but this is the first time
I can remember someone expressing a concern about a KDF.

Anbazhagan, or anyone, is there some reason to think that the PBKDF2
approach used by SCRAM is a problem?

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

[1] 
https://en.wikipedia.org/wiki/Salted_Challenge_Response_Authentication_Mechanism#Password-based_derived_key,_or_salted_password

Re: Proposal: Document ABI Compatibility

[David E. Wheeler]

On Jun 26, 2024, at 04:48, Laurenz Albe  wrote:

> Perhaps such information should go somewhere here:
> https://www.postgresql.org/support/versioning/

This seems deeper and more detailed than what’s there now, but I can certainly 
imagine wanting to include this policy on the web site. That said, it didn’t 
occur to me to look under support when trying to find a place to put this; I 
was looking under Developers, on the principle that extension developers would 
look there.

Best,

David

Re: psql (PostgreSQL) 17beta2 (Debian 17~beta2-1.pgdg+~20240625.1534.g23c5a0e) Failed to retrieve data from the server..

[Bruce Momjian]

On Wed, Jun 26, 2024 at 09:59:35AM -0400, Bruce Momjian wrote:
> On Wed, Jun 26, 2024 at 12:06:13AM -0400, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > On Tue, Jun 25, 2024 at 05:52:47PM -0700, David G. Johnston wrote:
> > >> We seem to be missing a release note item for the catalog breakage done 
> > >> in
> > >> f696c0c
> > >> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f696c0cd5f
> > 
> > > It seemed too internal to mention in the release notes --- more of an
> > > infrastructure change, but I can add it if I was wrong about this.
> > 
> > As this breakage demonstrates, that change is quite
> > application-visible.  It needs an entry under incompatibilities.
> 
> Okay, will do today.

Done.

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

  Only you can decide what is important to you.

Re: Reg: Alternate way of hashing database role passwords

[Tom Lane]

Robert Haas  writes:
> On Wed, Jun 26, 2024 at 12:11 PM Tom Lane  wrote:
>> It is not, and I doubt we have any interest in dramatically expanding
>> the set of allowed password hashes.  Adding SCRAM was enough work and
>> created a lot of client-v-server and cross-version incompatibility
>> already; nobody is in a hurry to repeat that.  Moreover, I know of
>> no reason to think that SHA-256 isn't perfectly adequate.

> If history is any guide, every algorithm will eventually look too
> weak. It seems inevitable that we're going to have to keep changing
> algorithms as time passes. However, it seems like SCRAM is designed so
> that different hash functions can be substituted into it, so what I'm
> hoping is that we can keep SCRAM and just replace SCRAM-SHA-256 with
> SCRAM-WHATEVER when SHA-256 starts to look too weak.

Totally agreed, that day will come.  What I'm pushing back on is the
suggestion that we should implement a ton of variant password hash
functionality on the basis of somebody's whim.  The costs are large
and they are not all paid by us, so the bar to replacing any part
of that has to be very high.

> What I find a bit surprising about Anbazhagan's question is that he
> asks about PBKDF2, which seems to be part of SCRAM already.[1] In
> fact, I think all the things he lists are key derivation functions,
> not hash functions.

This I don't have any info about.

regards, tom lane

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

[Nikolay Shaplov]

В письме от понедельник, 24 июня 2024 г. 23:51:56 MSK пользователь Nikolay 
Shaplov написал:

So,  I continue reading the patch.

I see there is `entries` variable in the code, that is the list of 
`RestrictInfo` objects and `entry` that is `OrClauseGroup` object.

This naming is quite misguiding (at least for me).

 `entries` variable name can be used, as we deal only with RestrictInfo 
entries here. It is kind of "generic" type. Though naming it 
`restric_info_entry` might make te code more readable.

But when we come to an `entry` variable, it is very specific entry, it should 
be `OrClauseGroup` entry, not just any entry. So I would suggest to name this 
variable `or_clause_group_entry`, or even `or_clause_group` , so when we meet 
this variable in the middle of the code, we can get the idea what we are 
dealing with, without scrolling code up.

Variable naming is very important thing. It can drastically improve (or ruin) 
code readability.



Also I see some changes in the tests int this patch. There are should be tests 
that check that this new feature works well. And there are test whose behavior 
have been just accidentally affected.

I whould suggest to split these tests into two patches, as they should be 
reviewed in different ways. Functionality tests should be thoroughly checked 
that all stuff we added is properly tested, and affected tests should be 
checked 
that nothing important is not broken. It would be more easy to check if these 
are two different patches.

I would also suggest to add to the commit message of affected tests changes 
some explanation why this changes does not really breaks anything. This will 
do the checking more simple.

To be continued.

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su


signature.asc
Description: This is a digitally signed message part.

JIT causes core dump during error recovery

[Tom Lane]

I initially ran into this while trying to reproduce the recent
reports of trouble with LLVM 14 on ARM.  However, it also reproduces
with LLVM 17 on x86_64, and I see no reason to think it's at all
arch-specific.  I also reproduced it in back branches (only tried
v14, but it's definitely not new in HEAD).

To reproduce:

1. Build with --with-llvm

2. Create a config file containing

$ cat $HOME/tmp/temp_config
# enable jit at max
jit_above_cost = 1
jit_inline_above_cost = 1
jit_optimize_above_cost = 1

and do
export TEMP_CONFIG=$HOME/tmp/temp_config

3. cd to .../src/pl/plpgsql/src/, and do "make check".

It gets a SIGSEGV in plpgsql_transaction.sql's
cursor_fail_during_commit test.  The stack trace looks like

(gdb) bt
#0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
#1  0x00735c58 in pq_sendstring (buf=0x7ffd80f8eeb0, 
str=0x7f77cffdf000 )
at pqformat.c:197
#2  0x009ca09c in err_sendstring (buf=0x7ffd80f8eeb0, 
str=0x7f77cffdf000 )
at elog.c:3449
#3  0x009ca4ba in send_message_to_frontend (edata=0xf786a0 )
at elog.c:3568
#4  0x009c73a3 in EmitErrorReport () at elog.c:1715
#5  0x008987e7 in PostgresMain (dbname=, 
username=0x29fdb00 "postgres") at postgres.c:4378
#6  0x00893c5d in BackendMain (startup_data=, 
startup_data_len=) at backend_startup.c:105

The errordata structure it's trying to print out contains

(gdb) p *edata
$1 = {elevel = 21, output_to_server = true, output_to_client = true, 
  hide_stmt = false, hide_ctx = false, 
  filename = 0x7f77cffdf000 , lineno = 843, 
  funcname = 0x7f77cffdf033 , domain = 0xbd3baa "postgres-17", 
  context_domain = 0x7f77c3343320 "plpgsql-17", sqlerrcode = 33816706, 
  message = 0x29fdc20 "division by zero", detail = 0x0, detail_log = 0x0, 
  hint = 0x0, 
  context = 0x29fdc50 "PL/pgSQL function cursor_fail_during_commit() line 6 at 
COMMIT", backtrace = 0x0, 
  message_id = 0x7f77cffdf022 , schema_name = 0x0, table_name = 0x0, column_name = 0x0, 
  datatype_name = 0x0, constraint_name = 0x0, cursorpos = 0, internalpos = 0, 
  internalquery = 0x0, saved_errno = 2, assoc_context = 0x29fdb20}

lineno = 843 matches the expected error location in int4_div().
The three string fields containing obviously-garbage pointers
are ones that elog.c expects to point at compile-time constants,
so it just records the caller's pointers without strdup'ing them.

Perhaps somebody else will know better, but what I think is happening
here is

A. Thanks to the low jit cost settings, we choose to jit-compile
the "1/(x-1000)" expression inside cursor_fail_during_commit().

B. When x reaches 1000, the division-by-zero error that the test
intends to provoke is thrown from jit-compiled code.

C. Somewhere between there and EmitErrorReport(), something decided
it could unmap the jit-compiled code.

D. Now filename/funcname are pointing into the void, and 
send_message_to_frontend() dumps core while trying to send them.

One way to fix this could be to pstrdup those strings even
though they should be constants.  I don't especially like
the amount of overhead that'd add though.

What I think is the right solution is to fix things so that
seemingly-no-longer-used jit compilations are not thrown away
until transaction cleanup.  I don't know the JIT code nearly
well enough to take point on fixing it like that, though.

regards, tom lane

Re: Patch bug: Fix jsonpath .* on Arrays

[David E. Wheeler]

On Jun 25, 2024, at 13:48, David E. Wheeler  wrote:

> I have since realized it’s not a complete fix for the issue, and hacked 
> around it in my Go version. Would be fine to remove that bit, but IIRC this 
> was the only execution function that would return `jperNotFound` when it in 
> fact adds items to the `found` list. The current implementation only looks at 
> one or the other, so it’s not super important, but I found the inconsistency 
> annoying and sometimes confusing.

I’ve removed this change.

>> [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
>> I propose adding a similar test with explicitly specified lax mode: select 
>> jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what lax mode 
>> is set by default.
> 
> Very few of the other tests do so; I can add it if it’s important for this 
> case, though.

Went ahead and added lax.

> @? suppresses a number of errors. Perhaps I should add a variant of the 
> error-raising query that passes the silent arg, which would also suppress the 
> error.

Added a variant where the silent param suppresses the error, too.

V2 attached and the PR updated:

  https://github.com/theory/postgres/pull/4/files

Best,

David



v2-0001-Add-tests-for-jsonpath-.-on-arrays.patch
Description: Binary data

Re: JIT causes core dump during error recovery

[Ranier Vilela]

Em qua., 26 de jun. de 2024 às 15:09, Tom Lane  escreveu:

> I initially ran into this while trying to reproduce the recent
> reports of trouble with LLVM 14 on ARM.  However, it also reproduces
> with LLVM 17 on x86_64, and I see no reason to think it's at all
> arch-specific.  I also reproduced it in back branches (only tried
> v14, but it's definitely not new in HEAD).
>
> To reproduce:
>
> 1. Build with --with-llvm
>
> 2. Create a config file containing
>
> $ cat $HOME/tmp/temp_config
> # enable jit at max
> jit_above_cost = 1
> jit_inline_above_cost = 1
> jit_optimize_above_cost = 1
>
> and do
> export TEMP_CONFIG=$HOME/tmp/temp_config
>
> 3. cd to .../src/pl/plpgsql/src/, and do "make check".
>
> It gets a SIGSEGV in plpgsql_transaction.sql's
> cursor_fail_during_commit test.  The stack trace looks like
>
> (gdb) bt
> #0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> #1  0x00735c58 in pq_sendstring (buf=0x7ffd80f8eeb0,
> str=0x7f77cffdf000  0x7f77cffdf000>)
> at pqformat.c:197
> #2  0x009ca09c in err_sendstring (buf=0x7ffd80f8eeb0,
> str=0x7f77cffdf000  0x7f77cffdf000>)
> at elog.c:3449
> #3  0x009ca4ba in send_message_to_frontend (edata=0xf786a0
> )
> at elog.c:3568
> #4  0x009c73a3 in EmitErrorReport () at elog.c:1715
> #5  0x008987e7 in PostgresMain (dbname=,
> username=0x29fdb00 "postgres") at postgres.c:4378
> #6  0x00893c5d in BackendMain (startup_data=,
> startup_data_len=) at backend_startup.c:105
>
> The errordata structure it's trying to print out contains
>
> (gdb) p *edata
> $1 = {elevel = 21, output_to_server = true, output_to_client = true,
>   hide_stmt = false, hide_ctx = false,
>   filename = 0x7f77cffdf000  0x7f77cffdf000>, lineno = 843,
>   funcname = 0x7f77cffdf033  0x7f77cffdf033>, domain = 0xbd3baa "postgres-17",
>   context_domain = 0x7f77c3343320 "plpgsql-17", sqlerrcode = 33816706,
>   message = 0x29fdc20 "division by zero", detail = 0x0, detail_log = 0x0,
>   hint = 0x0,
>   context = 0x29fdc50 "PL/pgSQL function cursor_fail_during_commit() line
> 6 at COMMIT", backtrace = 0x0,
>   message_id = 0x7f77cffdf022  0x7f77cffdf022>, schema_name = 0x0, table_name = 0x0, column_name = 0x0,
>   datatype_name = 0x0, constraint_name = 0x0, cursorpos = 0, internalpos =
> 0,
>   internalquery = 0x0, saved_errno = 2, assoc_context = 0x29fdb20}
>
> lineno = 843 matches the expected error location in int4_div().
>
Did you mean *int4div*, right?
Since there is no reference to int4_div in *.c or *.h

best regards,
Ranier Vilela

thread-safety: gmtime_r(), localtime_r()

[Peter Eisentraut]

Here is a patch for using gmtime_r() and localtime_r() instead of 
gmtime() and localtime(), for thread-safety.


There are a few affected calls in libpq and ecpg's libpgtypes, which are 
probably effectively bugs, because those libraries already claim to be 
thread-safe.


There is one affected call in the backend.  Most of the backend 
otherwise uses the custom functions pg_gmtime() and pg_localtime(), 
which are implemented differently.


Some portability fun: gmtime_r() and localtime_r() are in POSIX but are 
not available on Windows.  Windows has functions gmtime_s() and 
localtime_s() that can fulfill the same purpose, so we can add some 
small wrappers around them.  (Note that these *_s() functions are also

different from the *_s() functions in the bounds-checking extension of
C11.  We are not using those here.)

MinGW exposes neither *_r() nor *_s() by default.  You can get at the
POSIX-style *_r() functions by defining _POSIX_C_SOURCE appropriately
before including .  (There is apparently probably also a way to 
get at the Windows-style *_s() functions by supplying some additional 
options or defines.  But we might as well just use the POSIX ones.)From 4fa197f4beb0edabb426bb0a7e998e7dba0aacab Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 26 Jun 2024 20:37:02 +0200
Subject: [PATCH] thread-safety: gmtime_r(), localtime_r()

Use gmtime_r() and localtime_r() instead of gmtime() and localtime(),
for thread-safety.

There are a few affected calls in libpq and ecpg's libpgtypes, which
are probably effectively bugs, because those libraries already claim
to be thread-safe.

There is one affected call in the backend.  Most of the backend
otherwise uses the custom functions pg_gmtime() and pg_localtime(),
which are implemented differently.

Portability: gmtime_r() and localtime_r() are in POSIX but are not
available on Windows.  Windows has functions gmtime_s() and
localtime_s() that can fulfill the same purpose, so we add some small
wrappers around them.  (Note that these *_s() functions are also
different from the *_s() functions in the bounds-checking extension of
C11.  We are not using those here.)

MinGW exposes neither *_r() nor *_s() by default.  You can get at the
POSIX-style *_r() functions by defining _POSIX_C_SOURCE appropriately
before including .  (There is probably also a way to get at
the Windows-style *_s() functions by supplying some additional options
or defines.  But we might as well just use the POSIX ones.)
---
 src/backend/utils/adt/pg_locale.c  |  3 ++-
 src/include/port/win32_port.h  | 11 +++
 src/interfaces/ecpg/pgtypeslib/dt_common.c | 11 +++
 src/interfaces/ecpg/pgtypeslib/timestamp.c |  3 ++-
 src/interfaces/libpq/fe-trace.c|  3 ++-
 5 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/src/backend/utils/adt/pg_locale.c 
b/src/backend/utils/adt/pg_locale.c
index 7e5bb2b703a..55621e555b9 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -809,6 +809,7 @@ cache_locale_time(void)
char   *bufptr;
time_t  timenow;
struct tm  *timeinfo;
+   struct tm   timeinfobuf;
boolstrftimefail = false;
int encoding;
int i;
@@ -859,7 +860,7 @@ cache_locale_time(void)
 
/* We use times close to current time as data for strftime(). */
timenow = time(NULL);
-   timeinfo = localtime(&timenow);
+   timeinfo = localtime_r(&timenow, &timeinfobuf);
 
/* Store the strftime results in MAX_L10N_DATA-sized portions of buf[] 
*/
bufptr = buf;
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 3d1de166cb0..6d5b8b50f23 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -415,6 +415,17 @@ extern int _pglstat64(const char *name, struct stat *buf);
 #undef ETIMEDOUT
 #define ETIMEDOUT WSAETIMEDOUT
 
+/*
+ * Supplement to .
+ */
+#ifdef _MSC_VER
+#define gmtime_r(clock, result) (gmtime_s(result, clock) ? NULL : (result))
+#define localtime_r(clock, result) (localtime_s(result, clock) ? NULL : 
(result))
+#else
+/* define before including  for getting localtime_r() etc. on MinGW */
+#define _POSIX_C_SOURCE 1
+#endif
+
 /*
  * Locale stuff.
  *
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c 
b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index ed08088bfe1..48074fd3ad1 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -949,9 +949,10 @@ int
 GetEpochTime(struct tm *tm)
 {
struct tm  *t0;
+   struct tm   tmbuf;
time_t  epoch = 0;
 
-   t0 = gmtime(&epoch);
+   t0 = gmtime_r(&epoch, &tmbuf);
 
if (t0)
{
@@ -973,12 +974,13 @@ abstime2tm(AbsoluteTime _time, int *tzp, struct tm *tm, 
char **tzn)
 {
time_t  time = (time_t) _time;
struct tm  *tx;
+   struct

Re: JIT causes core dump during error recovery

[Tom Lane]

Ranier Vilela  writes:
> Em qua., 26 de jun. de 2024 às 15:09, Tom Lane  escreveu:
>> lineno = 843 matches the expected error location in int4_div().

> Did you mean *int4div*, right?

Right, typo.

regards, tom lane

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

[Noah Misch]

On Wed, Jun 26, 2024 at 02:09:58PM +0300, Aleksander Alekseev wrote:
> > This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> > 32-bit integers" and left some expressions coercing SLRU page numbers to 
> > int.
> > Two sources:
> >
> >  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
> >  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 
> > 2>&1 | less -p '.int. from .int64. may alter its value'
> >
> > (Not every match needs to change.)
> 
> I examined the new warnings introduced by 4ed8f09. Most of them seem
> to be harmless, for instance:
[...]
> I prepared the patch for clog.c. The rest of the warnings don't strike
> me as something we should immediately act on unless we have a bug
> report. Or perhaps there is a particular warning that worries you?

Is "int" acceptable or unacceptable in the following grep match?

src/backend/commands/async.c:1274:  int headPage = 
QUEUE_POS_PAGE(QUEUE_HEAD);

Re: JIT causes core dump during error recovery

[Tom Lane]

I wrote:
> It gets a SIGSEGV in plpgsql_transaction.sql's
> cursor_fail_during_commit test.

Here's a simpler way to reproduce: just run the attached script
in a --with-llvm build.  (This is merely extracting the troublesome
regression case for convenience.)

Interesting, if you take out any one of the three "set" commands,
it doesn't crash.  This probably explains why, for example,
buildfarm member urutu hasn't shown this --- it's only reducing
one of the three costs to zero.

I don't have any idea what to make of that result, except that
it suggests the problem might be at least partly LLVM's fault.
Surely, if we are prematurely unmapping a compiled code segment,
that behavior wouldn't depend on whether we had asked for
inlining?

regards, tom lane

drop table if exists test1;
create table test1 (x int);

CREATE OR REPLACE PROCEDURE cursor_fail_during_commit()
 LANGUAGE plpgsql
AS $$
  DECLARE id int;
  BEGIN
FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
INSERT INTO test1 VALUES(id);
COMMIT;
END LOOP;
  END;
$$;

set jit_above_cost=0;
set jit_optimize_above_cost=0;
set jit_inline_above_cost=0;

CALL cursor_fail_during_commit();

Re: Proposal: Document ABI Compatibility

[David E. Wheeler]

On Jun 25, 2024, at 13:55, David E. Wheeler  wrote:

> Oh man this is fantastic, thank you! I’d be more than happy to just turn this 
> into a patch. But where should it go? Upthread I assumed xfunc.sgml, and 
> still think that’s a likely candidate. Perhaps I’ll just start there --- 
> unless someone thinks it should go somewhere other than the docs.

Okay here’s a patch that adds the proposed API and ABI guidance to the C 
Language docs. The content is the same as Peter proposed, with some light 
copy-editing.

Best,

David



v1-0001-Add-API-an-ABI-guidance-to-the-C-language-docs.patch
Description: Binary data

Re: Proposal: Document ABI Compatibility

[David E. Wheeler]

On Jun 26, 2024, at 15:14, David E. Wheeler  wrote:

> Okay here’s a patch that adds the proposed API and ABI guidance to the C 
> Language docs. The content is the same as Peter proposed, with some light 
> copy-editing.

CF: https://commitfest.postgresql.org/48/5080/
PR: https://github.com/theory/postgres/pull/6

D

Re: speed up a logical replica setup

[Noah Misch]

On Tue, Jun 25, 2024 at 09:50:59PM -0300, Euler Taveira wrote:
> On Tue, Jun 25, 2024, at 3:24 AM, Amit Kapila wrote:
> > On Tue, Jun 25, 2024 at 3:38 AM Noah Misch  wrote:
> > > On Mon, Jun 24, 2024 at 05:20:21PM +0530, Amit Kapila wrote:
> > > > On Sun, Jun 23, 2024 at 11:52 AM Noah Misch  wrote:
> > > > > > +static void
> > > > > > +create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
> > > > > > +{
> > > > >
> > > > > > + appendPQExpBuffer(str, "CREATE PUBLICATION %s FOR ALL TABLES",
> > > > > > +   ipubname_esc);
> > > > >
> > > > > This tool's documentation says it "guarantees that no transaction 
> > > > > will be
> > > > > lost."  I tried to determine whether achieving that will require 
> > > > > something
> > > > > like the fix from
> > > > > https://postgr.es/m/flat/de52b282-1166-1180-45a2-8d8917ca7...@enterprisedb.com.
> > > > > (Not exactly the fix from that thread, since that thread has not 
> > > > > discussed the
> > > > > FOR ALL TABLES version of its race condition.)  I don't know.  On the 
> > > > > one
> > > > > hand, pg_createsubscriber benefits from creating a logical slot after 
> > > > > creating
> > > > > the publication.  That snapbuild.c process will wait for running 
> > > > > XIDs.  On the
> > > > > other hand, an INSERT/UPDATE/DELETE acquires its RowExclusiveLock and 
> > > > > builds
> > > > > its relcache entry before assigning an XID, so perhaps the 
> > > > > snapbuild.c process
> > >
> > > Correction: it doesn't matter how the original INSERT/UPDATE/DELETE 
> > > builds its
> > > relcache entry, just how pgoutput of the change builds the relcache entry 
> > > from
> > > the historic snapshot.
> > >
> > > > > isn't enough to prevent that thread's race condition.  What do you 
> > > > > think?
> > > >
> > > > I am not able to imagine how the race condition discussed in the
> > > > thread you quoted can impact this patch. The problem discussed is
> > > > mainly the interaction when we are processing the changes in logical
> > > > decoding w.r.t concurrent DDL (Alter Publication ... Add Table). The
> > > > problem happens because we use the old cache state.
> > >
> > > Right.  Taking the example from
> > > http://postgr.es/m/20231119021830.d6t6aaxtrkpn7...@awork3.anarazel.de, 
> > > LSNs
> > > between what that mail calls 4) and 5) are not safely usable as start 
> > > points.
> > > pg_createsubscriber evades that thread's problem if the consistent_lsn it
> > > passes to pg_replication_origin_advance() can't be in a bad-start-point 
> > > LSN
> > > span.  I cautiously bet the snapbuild.c process achieves that:
> > >
> > > > I am missing your
> > > > point about the race condition mentioned in the thread you quoted with
> > > > snapbuild.c. Can you please elaborate a bit more?
> > >
> > > When pg_createsubscriber calls pg_create_logical_replication_slot(), the 
> > > key
> > > part starts at:
> > >
> > > /*
> > >  * If caller needs us to determine the decoding start point, do 
> > > so now.
> > >  * This might take a while.
> > >  */
> > > if (find_startpoint)
> > > DecodingContextFindStartpoint(ctx);
> > >
> > > Two factors protect pg_createsubscriber.  First, (a) CREATE PUBLICATION
> > > committed before pg_create_logical_replication_slot() started.  Second, 
> > > (b)
> > > DecodingContextFindStartpoint() waits for running XIDs to complete, via 
> > > the
> > > process described at the snapbuild.c "starting up in several stages" 
> > > diagram.
> > > Hence, the consistent_lsn is not in a bad-start-point LSN span.  It's fine
> > > even if the original INSERT populated all caches before CREATE PUBLICATION
> > > started and managed to assign an XID only after consistent_lsn.  From the
> > > pgoutput perspective, that's indistinguishable from the transaction 
> > > starting
> > > at its first WAL record, after consistent_lsn.  The linked "long-standing 
> > > data
> > > loss bug in initial sync of logical replication" thread doesn't have (a),
> > > hence its bug.  How close is that to accurate?
> > 
> > Yeah, this theory sounds right to me. The key point is that no DML
> > (processing of WAL corresponding to DML) before CREATE PUBLICATION ...
> > command would have reached pgoutput level because we would have waited
> > for it during snapbuild.c. Can we conclude that the race condition
> > discussed in the other thread won't impact this patch?
> 
> As Noah said the key point is the CREATE PUBLICATION *before* creating the
> replication slots -- that wait transactions to complete.

Let's consider the transaction loss topic concluded.  Thanks for contemplating
it.  I've added an open item for the "dbname containing a space" topic.

Re: Proposal: Document ABI Compatibility

[David E. Wheeler]

On Jun 26, 2024, at 15:20, David E. Wheeler  wrote:

> CF: https://commitfest.postgresql.org/48/5080/
> PR: https://github.com/theory/postgres/pull/6

Aaaand v2 without the unnecessary formatting of unrelated documentation 🤦🏻‍♂️.

Best,

David



v2-0001-Add-API-an-ABI-guidance-to-the-C-language-docs.patch
Description: Binary data

Re: New standby_slot_names GUC in PG 17

[Peter Eisentraut]

On 21.06.24 17:37, Bruce Momjian wrote:

The release notes have this item:

Allow specification of physical standbys that must be synchronized
before they are visible to subscribers (Hou Zhijie, Shveta Malik)

The new server variable is standby_slot_names.

Is standby_slot_names an accurate name for this GUC?  It seems too
generic.


This was possibly inspired by pg_failover_slots.standby_slot_names 
(which in turn came from pglogical.standby_slot_names).  In those cases, 
you have some more context from the extension prefix.


The new suggested names sound good to me.

Re: JIT causes core dump during error recovery

[Tom Lane]

I wrote:
> What I think is the right solution is to fix things so that
> seemingly-no-longer-used jit compilations are not thrown away
> until transaction cleanup.  I don't know the JIT code nearly
> well enough to take point on fixing it like that, though.

Or maybe not.  I found by bisecting that it doesn't fail before
2e517818f (Fix SPI's handling of errors during transaction commit).
A salient part of that commit message:

Having made that API redefinition, we can fix this mess by having
SPI_commit[_and_chain] trap errors and start a new, clean transaction
before re-throwing the error.  Likewise for SPI_rollback[_and_chain].

So delaying removal of the jit-created code segment until transaction
cleanup wouldn't be enough to prevent this crash, if I'm reading
things right.  The extra-pstrdup solution may be the only viable one.

I could use confirmation from someone who knows the JIT code about
when jit-created code is unloaded.  It also remains very unclear
why there is no crash if we don't force both jit_optimize_above_cost
and jit_inline_above_cost to small values.

regards, tom lane

Re: libpq: Fix lots of discrepancies in PQtrace

[Jelte Fennema-Nio]

On Wed, 26 Jun 2024 at 18:36, Alvaro Herrera  wrote:
> Thanks, Nathan.  I'm holding myself responsible for the rest ... will
> handle soon after the branch.

Sounds great. Out of curiosity, what is the backpatching policy for
something like this? Honestly most of these patches could be
considered bugfixes in PQtrace, so backpatching might make sense. OTOH
I don't think PQtrace is used very much in practice, so backpatching
might carry more risk than it's worth.

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

[Robert Haas]

I think maybe replying to multiple emails with a single email is
something you'd be better off doing less often.

On Tue, Jun 25, 2024 at 7:14 PM Alena Rybakina
 wrote:
> Sorry, you are right and I'll try to explain more precisely. The first 
> approach is the first part of the patch, where we made "Or" expressions into 
> an SAOP at an early stage of plan generation [0], the second one was the one 
> proposed by A.Korotkov [1].

[0] isn't doing anything "at an early stage of plan generation".  It's
changing something in *the parser*. The parser and planner are VERY
different stages of query parsing, and it's really important to keep
them separate mentally and in discussions. We should not be changing
anything about the query in the parser, because that will, as
Alexander also pointed out, change what gets stored if the user does
something like CREATE VIEW whatever AS SELECT ...; and we should, for
the most part, be storing the query as the user entered it, not some
transformed version of it. Further, at the parser stage, we do not
know the cost of anything, so we can only transform things when the
transformed version is always - or practically always - going to be
cheaper than the untransformed version.

> On 24.06.2024 18:28, Robert Haas wrote:
> Andrei mentioned the problem, which might be caused by the transformation on 
> the later stage of optimization is updating references to expressions in 
> RestrictInfo [3] lists, because they can be used in different parts during 
> the formation of the query plan. As the practice of Self-join removal [4] has 
> shown, this can be expensive, but feasible. By applying the transformation at 
> the analysis stage [0], because no links were created, so we did not 
> encounter such problems, so this approach was more suitable than the others.

The link you provided for [3] doesn't show me exactly what code you're
talking about, but I can see why mutating a RestrictInfo after
creating it could be problematic. However, I'm not proposing that, and
I don't think it's a good idea. Instead of mutating an existing data
structure after it's been created, we want to get each data structure
correct at the moment that it is created. What that means is that at
each stage of processing, whenever we create a new in-memory data
structure, we have an opportunity to make changes along the way.

For instance, let's say we have a RestrictInfo and we are creating a
Path, perhaps via create_index_path(). One argument to that function
is a list of indexclauses. The indexclauses are derived from the
RestrictInfo list associated with the RelOptInfo. We take some subset
of those quals that are deemed to be indexable and we reorder them and
maybe change a few things and we build this new list of indexclauses
that is then passed to create_index_path(). The RelOptInfo's list of
RestrictInfos is not changed -- only the new list of clauses derived
from it is being built up here, without any mutation of the original
structure.

This is the kind of thing that this patch can and probably should do.
Join removal is quite awkward, as you rightly point out, because we
end up modifying existing data structures after they've been created,
and that requires us to run around and fix up a bunch of stuff, and
that can have bugs. Whenever possible, we don't want to do it that
way. Instead, we want to pick points in the processing when we're
anyway constructing some new structure and use that as an opportunity
to do transformations when building the new structure that incorporate
optimizations that make sense.

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

Re: Recommended books for admin

[Tom Browder]

On Tue, Jun 25, 2024 at 9:15 AM Muhammad Ikram  wrote:
> Hi ,
> Here is a lately published book
> https://www.amazon.com/PostgreSQL-Administration-Cookbook-real-world-challenges-ebook/dp/B0CP5PPSTQ

Thanks, Muhammed, I just bought it.

And thanks to all who answered!

Best regards.

-Tom

Re: TruncateMultiXact() bugs

[Heikki Linnakangas]

On 14/06/2024 14:37, Heikki Linnakangas wrote:

I was performing tests around multixid wraparound, when I ran into this
assertion:


TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: 
"../src/backend/utils/mmgr/mcxt.c", Line: 1353, PID: 920981
postgres: autovacuum worker template0(ExceptionalCondition+0x6e)[0x560a501e866e]
postgres: autovacuum worker template0(+0x5dce3d)[0x560a50217e3d]
postgres: autovacuum worker template0(ForwardSyncRequest+0x8e)[0x560a4ffec95e]
postgres: autovacuum worker template0(RegisterSyncRequest+0x2b)[0x560a50091eeb]
postgres: autovacuum worker template0(+0x187b0a)[0x560a4fdc2b0a]
postgres: autovacuum worker template0(SlruDeleteSegment+0x101)[0x560a4fdc2ab1]
postgres: autovacuum worker template0(TruncateMultiXact+0x2fb)[0x560a4fdbde1b]
postgres: autovacuum worker 
template0(vac_update_datfrozenxid+0x4b3)[0x560a4febd2f3]
postgres: autovacuum worker template0(+0x3adf66)[0x560a4ffe8f66]
postgres: autovacuum worker template0(AutoVacWorkerMain+0x3ed)[0x560a4ffe7c2d]
postgres: autovacuum worker template0(+0x3b1ead)[0x560a4ffecead]
postgres: autovacuum worker template0(+0x3b620e)[0x560a4fff120e]
postgres: autovacuum worker template0(+0x3b3fbb)[0x560a4ffeefbb]
postgres: autovacuum worker template0(+0x2f724e)[0x560a4ff3224e]
/lib/x86_64-linux-gnu/libc.so.6(+0x27c8a)[0x7f62cc642c8a]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85)[0x7f62cc642d45]
postgres: autovacuum worker template0(_start+0x21)[0x560a4fd16f31]
2024-06-14 13:11:02.025 EEST [920971] LOG:  server process (PID 920981) was 
terminated by signal 6: Aborted
2024-06-14 13:11:02.025 EEST [920971] DETAIL:  Failed process was running: 
autovacuum: VACUUM pg_toast.pg_toast_13407 (to prevent wraparound)


The attached python script reproduces this pretty reliably. It's a
reduced version of a larger test script I was working on, it probably
could be simplified further for this particular issue.

Looking at the code, it's pretty clear how it happens:

1. TruncateMultiXact does START_CRIT_SECTION();

2. In the critical section, it calls PerformMembersTruncation() ->
SlruDeleteSegment() -> SlruInternalDeleteSegment() ->
RegisterSyncRequest() -> ForwardSyncRequest()

3. If the fsync request queue is full, it calls
CompactCheckpointerRequestQueue(), which calls palloc0. Pallocs are not
allowed in a critical section.

A straightforward fix is to add a check to
CompactCheckpointerRequestQueue() to bail out without compacting, if
it's called in a critical section. That would cover any other cases like
this, where RegisterSyncRequest() is called in a critical section. I
haven't tried searching if any more cases like this exist.

But wait there is more!

After applying that fix in CompactCheckpointerRequestQueue(), the test
script often gets stuck. There's a deadlock between the checkpointer,
and the autovacuum backend trimming the SLRUs:

1. TruncateMultiXact does this:

MyProc->delayChkptFlags |= DELAY_CHKPT_START;

2. It then makes that call to PerformMembersTruncation() and
RegisterSyncRequest(). If it cannot queue the request, it sleeps a
little and retries. But the checkpointer is stuck waiting for the
autovacuum backend, because of delayChkptFlags, and will never clear the
queue.

To fix, I propose to add AbsorbSyncRequests() calls to the wait-loops in
CreateCheckPoint().


Attached patch fixes both of those issues.


Committed and backpatched down to v14. This particular scenario cannot 
happen in older versions because the RegisterFsync() on SLRU truncation 
was added in v14. In principle, I think older versions might have 
similar issues, but given that when assertions are disabled this is only 
a problem if you happen to run out of memory in the critical section, it 
doesn't seem worth backpatching further unless someone reports a 
concrete case.


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

Re: Should we document how column DEFAULT expressions work?

[David Rowley]

On Wed, 26 Jun 2024 at 11:05, James Coleman  wrote:
> Hmm, I guess I'd never considered anything besides cases like
> nextval() and now(), but I see now that now() must also be special
> cased (when quoted) since 'date_trunc(day, now())'::timestamp doesn't
> work but 'now()'::timestamp does.

'now()'::timestamp only works because we ignore trailing punctuation
in ParseDateTime() during timestamp_in(). 'now!!'::timestamp works
equally as well.

David

Re: Should we document how column DEFAULT expressions work?

[David G. Johnston]

On Tuesday, June 25, 2024, James Coleman  wrote:

> Hello,
>
> It's possible I'm the only one who's been in this situation, but I've
> multiple times found myself explaining to a user how column DEFAULT
> expressions work: namely how the quoting on an expression following
> the keyword DEFAULT controls whether or not the expression is
> evaluated at the time of the DDL statement or at the time of an
> insertion.
>

I don’t know if it’s worth documenting but the following sentence is
implied by the syntax:

“Do not single quote the expression as a whole.  Write the expression as
you would in a select query.”

David J.

Re: Should we document how column DEFAULT expressions work?

[David Rowley]

On Wed, 26 Jun 2024 at 17:12, Tom Lane  wrote:
> Do we want to do anything about
> nextval()?  I guess if you hold your head at the correct
> angle, that's also a magic-input-value issue, in the sense
> that the question is when does regclass input get resolved.

I think I'm not understanding what's special about that.  Aren't
'now'::timestamp and 'seq_name'::regclass are just casts that are
evaluated during parse time in transformExpr()?

David

Re: Should we document how column DEFAULT expressions work?

[Tom Lane]

David Rowley  writes:
> On Wed, 26 Jun 2024 at 17:12, Tom Lane  wrote:
>> Do we want to do anything about
>> nextval()?  I guess if you hold your head at the correct
>> angle, that's also a magic-input-value issue, in the sense
>> that the question is when does regclass input get resolved.

> I think I'm not understanding what's special about that.  Aren't
> 'now'::timestamp and 'seq_name'::regclass are just casts that are
> evaluated during parse time in transformExpr()?

Right.  But there is an example in the manual explaining how
these two things act differently:

'seq_name'::regclass
'seq_name'::text::regclass

The latter produces a constant of type text with a run-time
cast to regclass (and hence a run-time pg_class lookup).
IIRC, we document that mainly because the latter provides a way
to duplicate nextval()'s old behavior of run-time lookup.

Now that I think about it, there's a very parallel difference in
the behavior of

'now'::timestamp
'now'::text::timestamp

but I doubt that that example is shown anywhere.

regards, tom lane

Re: Should we document how column DEFAULT expressions work?

[David Rowley]

On Wed, 26 Jun 2024 at 17:36, David G. Johnston
 wrote:
>
> On Tue, Jun 25, 2024 at 9:50 PM David Rowley  wrote:
>> FWIW, I disagree that we need to write anything about that in this
>> part of the documentation.  I think any argument for doing this could
>> equally be applied to something like re-iterating what the operator
>> precedence rules for arithmetic are, and I don't think that should be
>> mentioned.
>
>
> I disagree on this equivalence.  The time literals are clear deviations from 
> expected behavior.  Knowing operator precedence rules, they apply everywhere 
> equally.  And we should document the deviations directly where they happen.  
> Even if it's just a short link back to the source that describes the 
> deviation.  I'm fine with something less verbose pointing only to the data 
> types page, but not with nothing.

Are you able to share what the special behaviour is with DEFAULT
constraints and time literals that does not apply everywhere equally?

Maybe I'm slow on the uptake, but I've yet to see anything here where
time literals act in a special way DEFAULT constraints. This is why I
couldn't understand why we should be adding documentation about this
under CREATE TABLE.

I'd be happy to reconsider or retract my argument if you can show me
what I'm missing.

David

Re: Should we document how column DEFAULT expressions work?

[Tom Lane]

David Rowley  writes:
> Maybe I'm slow on the uptake, but I've yet to see anything here where
> time literals act in a special way DEFAULT constraints. This is why I
> couldn't understand why we should be adding documentation about this
> under CREATE TABLE.

It's not that the parsing rules are any different: it's that in
ordinary DML queries, it seldom matters very much whether a
subexpression is evaluated at parse time versus run time.
In CREATE TABLE that difference is very in-your-face, so people
who haven't understood the rules clearly can get burnt.

However, there are certainly other places where it matters,
such as queries in plpgsql functions.  So I understand your
reluctance to go on about it in CREATE TABLE.  At the same
time, I see where David J. is coming from.

Maybe we could have a discussion of this in some single spot,
and link to it from CREATE TABLE and other relevant places?
ISTR there is something about it in the plpgsql doco already.

regards, tom lane

Re: Should we document how column DEFAULT expressions work?

[David Rowley]

On Thu, 27 Jun 2024 at 12:11, Tom Lane  wrote:
>
> David Rowley  writes:
> > Maybe I'm slow on the uptake, but I've yet to see anything here where
> > time literals act in a special way DEFAULT constraints. This is why I
> > couldn't understand why we should be adding documentation about this
> > under CREATE TABLE.
>
> It's not that the parsing rules are any different: it's that in
> ordinary DML queries, it seldom matters very much whether a
> subexpression is evaluated at parse time versus run time.
> In CREATE TABLE that difference is very in-your-face, so people
> who haven't understood the rules clearly can get burnt.

Aha, now I understand. Thanks. So, seems like CREATE TABLE is being
targeted or maybe victimised here as it's probably the most common
place people learn about their misuse of the timestamp special input
values.

> However, there are certainly other places where it matters,
> such as queries in plpgsql functions.  So I understand your
> reluctance to go on about it in CREATE TABLE.  At the same
> time, I see where David J. is coming from.
>
> Maybe we could have a discussion of this in some single spot,
> and link to it from CREATE TABLE and other relevant places?
> ISTR there is something about it in the plpgsql doco already.

For the special timestamp stuff, that place is probably the special
timestamp table in [1].  It looks like the large caution you added in
540849814 might not be enough or perhaps wasn't done soon enough to
catch the people who read that part of the manual before the caution
was added. Hard to fix if it's the latter without a time machine. :-(

I'm open to having some section that fleshes this stuff out a bit more
with a few examples with CREATE TABLE and maybe CREATE VIEW that we
can link to.  Linking seems like a much more sustainable practice than
adding special case documentation for non-special case behaviour.

David

[1] https://www.postgresql.org/docs/devel/datatype-datetime.html

Re: Fix possible overflow of pg_stat DSA's refcnt

[Michael Paquier]

On Wed, Jun 26, 2024 at 08:48:06AM +0200, Anthonin Bonnefoy wrote:
> Yeah, this happened last week on one of our replicas (version 15.5)
> last week that had 134 days uptime. We are doing a lot of parallel
> queries on this cluster so the combination of high uptime plus
> parallel workers creation eventually triggered the issue.

It is not surprising that it would take this much amount of time
before detecting it.  I've applied the patch down to 15.  Thanks a lot
for the analysis and the patch!
--
Michael


signature.asc
Description: PGP signature

Re: Add LSN <-> time conversion functionality

[Melanie Plageman]

On Mon, Mar 18, 2024 at 1:29 PM Tomas Vondra
 wrote:
>
> On 2/22/24 03:45, Melanie Plageman wrote:
> > The attached v3 has a new algorithm. Now, LSNTimes are added from the
> > end of the array forward until all array elements have at least one
> > logical member (array length == volume). Once array length == volume,
> > new LSNTimes will result in merging logical members in existing
> > elements. We want to merge older members because those can be less
> > precise. So, the number of logical members per array element will
> > always monotonically increase starting from the beginning of the array
> > (which contains the most recent data) and going to the end. We want to
> > use all the available space in the array. That means that each LSNTime
> > insertion will always result in a single merge. We want the timeline
> > to be inclusive of the oldest data, so merging means taking the
> > smaller value of two LSNTime values. I had to pick a rule for choosing
> > which elements to merge. So, I choose the merge target as the oldest
> > element whose logical membership is < 2x its predecessor. I merge the
> > merge target's predecessor into the merge target. Then I move all of
> > the intervening elements down 1. Then I insert the new LSNTime at
> > index 0.
> >
>
> I can't help but think about t-digest [1], which also merges data into
> variable-sized buckets (called centroids, which is a pair of values,
> just like LSNTime). But the merging is driven by something called "scale
> function" which I found like a pretty nice approach to this, and it
> yields some guarantees regarding accuracy. I wonder if we could do
> something similar here ...
>
> The t-digest is a way to approximate quantiles, and the default scale
> function is optimized for best accuracy on the extremes (close to 0.0
> and 1.0), but it's possible to use scale functions that optimize only
> for accuracy close to 1.0.
>
> This may be misguided, but I see similarity between quantiles and what
> LSNTimeline does - timestamps are ordered, and quantiles close to 0.0
> are "old timestamps" while quantiles close to 1.0 are "now".
>
> And t-digest also defines a pretty efficient algorithm to merge data in
> a way that gradually combines older "buckets" into larger ones.

I started taking a look at this paper and think the t-digest could be
applicable as a possible alternative data structure to the one I am
using to approximate page age for the actual opportunistic freeze
heuristic -- especially since the centroids are pairs of a mean and a
count. I couldn't quite understand how the t-digest is combining those
centroids. Since I am not computing quantiles over the LSNTimeStream,
though, I think I can probably do something simpler for this part of
the project.

> >> - The LSNTimeline comment claims an array of size 64 is large enough to
> >> not need to care about filling it, but maybe it should briefly explain
> >> why we can never fill it (I guess 2^64 is just too many).
-- snip --
> I guess that should be enough for (2^64-1) logical members, because it's
> a sequence 1, 2, 4, 8, ..., 2^63. Seems enough.
>
> But now that I think about it, does it make sense to do the merging
> based on the number of logical members? Shouldn't this really be driven
> by the "length" of the time interval the member covers?

After reading this, I decided to experiment with a few different
algorithms in python and plot the unabbreviated LSNTimeStream against
various ways of compressing it. You can see the results if you run the
python code here [1].

What I found is that attempting to calculate the error represented by
dropping a point and picking the point which would cause the least
additional error were it to be dropped produced more accurate results
than combining the oldest entries based on logical membership to fit
some series.

This is inspired by what you said about using the length of segments
to decide which points to merge. In my case, I want to merge segments
that have a similar slope -- those which have a point that is
essentially redundant. I loop through the LSNTimeStream and look for
the point that we can drop with the lowest impact on future
interpolation accuracy. To do this, I calculate the area of the
triangle made by each point on the stream and its adjacent points. The
idea being that if you drop that point, the triangle is the amount of
error you introduce for points being interpolated between the new pair
(previous adjacencies of the dropped point). This has some issues, but
it seems more logical than just always combining the oldest points.

If you run the python simulator code, you'll see that for the
LSNTimeStream I generated, using this method produces more accurate
results than either randomly dropping points or using the "combine
oldest members" method.

It would be nice if we could do something with the accumulated error
-- so we could use it to modify estimates when interpolating. I don't
really know how to keep it though. I thought I would 

Re: Vacuum statistics

[Masahiko Sawada]

On Fri, May 31, 2024 at 4:19 AM Andrei Zubkov  wrote:
>
> Hi,
>
> Th, 30/05/2024 at 10:33 -0700, Alena Rybakina wrote:
> > I suggest gathering information about vacuum resource consumption for
> > processing indexes and tables and storing it in the table and index
> > relationships (for example, PgStat_StatTabEntry structure like it has
> > realized for usual statistics). It will allow us to determine how
> > well
> > the vacuum is configured and evaluate the effect of overhead on the
> > system at the strategic level, the vacuum has gathered this
> > information
> > already, but this valuable information doesn't store it.
> >
> It seems a little bit unclear to me, so let me explain a little the
> point of a proposition.
>
> As the vacuum process is a backend it has a workload instrumentation.
> We have all the basic counters available such as a number of blocks
> read, hit and written, time spent on I/O, WAL stats and so on.. Also,
> we can easily get some statistics specific to vacuum activity i.e.
> number of tuples removed, number of blocks removed, number of VM marks
> set and, of course the most important metric - time spent on vacuum
> operation.

I've not reviewed the patch closely but it sounds helpful for users. I
would like to add a statistic, the high-water mark of memory usage of
dead tuple TIDs. Since the amount of memory used by TidStore is hard
to predict, I think showing the high-water mark would help users to
predict how much memory they set to maintenance_work_mem.

Regards,

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

Re: New standby_slot_names GUC in PG 17

[Masahiko Sawada]

On Wed, Jun 26, 2024 at 6:15 PM Zhijie Hou (Fujitsu)
 wrote:
>
> On Wednesday, June 26, 2024 12:49 PM Bertrand Drouvot 
>  wrote:
> >
> > Hi,
> >
> > On Wed, Jun 26, 2024 at 04:17:45AM +, Zhijie Hou (Fujitsu) wrote:
> > > On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada
> >  wrote:
> > > >
> > > > On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila
> > > > 
> > > > wrote:
> > > > >
> > > > > I feel synchronized better indicates the purpose because we ensure
> > > > > such slots are synchronized before we process changes for logical
> > > > > failover slots. We already have a 'failover' option for logical
> > > > > slots which could make things confusing if we add 'failover' where
> > > > > physical slots need to be specified.
> > > >
> > > > Agreed. So +1 for synchronized_stnadby_slots.
> > >
> > > +1.
> > >
> > > Since there is a consensus on this name, I am attaching the patch to
> > > rename the GUC to synchronized_stnadby_slots. I have confirmed that
> > > the regression tests and pgindent passed for the patch.
> > A few comments:
>
> Thanks for the comments!
>
> > 1 
> >
> > In the commit message:
> >
> > "
> > The standby_slot_names GUC is intended to allow specification of physical
> > standby slots that must be synchronized before they are visible to
> > subscribers
> > "
> >
> > Not sure that wording is correct, if we feel the need to explain the GUC, 
> > maybe
> > repeat some wording from bf279ddd1c?
>
> I intentionally copied some words from release note of this GUC which was
> also part of the content in the initial email of this thread. I think it
> would be easy to understand than the original commit msg. But others may
> have different opinion, so I would leave the decision to the committer. (I 
> adjusted
> a bit the word in this version).
>
> >
> > 2 
> >
> > Should we rename StandbySlotNamesConfigData too?
> >
> > 3 
> >
> > Should we rename SlotExistsInStandbySlotNames too?
> >
> > 4 
> >
> > Should we rename validate_standby_slots() too?
> >
>
> Renamed these to the names suggested by Amit.
>
> Attach the v2 patch set which addressed above and removed
> the changes in release-17.sgml according to the comment from Amit.
>

Thank you for updating the patch. The v2 patch looks good to me.

Regards,

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

Re: Add LSN <-> time conversion functionality

[Melanie Plageman]

Thanks for the review!

Attached v4 implements the new algorithm/compression described in [1].

We had discussed off-list possibly using error in some way. So, I'm
interested to know what you think about this method I suggested which
calculates error. It doesn't save the error so that we could use it
when interpolating for reasons I describe in that mail. If you have
any ideas on how to use the calculated error or just how to combine
error when dropping a point, that would be super helpful.

Note that in this version, I've changed the name from LSNTimeline to
LSNTimeStream to address some feedback from another reviewer about
Timeline being already in use in Postgres as a concept.

On Mon, Mar 18, 2024 at 10:02 AM Daniel Gustafsson  wrote:
>
> > On 22 Feb 2024, at 03:45, Melanie Plageman  
> > wrote:
> > On Fri, Feb 16, 2024 at 3:41 PM Tomas Vondra
> >  wrote:
> >> - I wonder what happens if we lose the data - we know that if people
> >> reset statistics for whatever reason (or just lose them because of a
> >> crash, or because they're on a replica), bad things happen to
> >> autovacuum. What's the (expected) impact on pruning?
> >
> > This is an important question. Because stats aren't crashsafe, we
> > could return very inaccurate numbers for some time/LSN values if we
> > crash. I don't actually know what we could do about that. When I use
> > the LSNTimeline for the freeze heuristic it is less of an issue
> > because the freeze heuristic has a fallback strategy when there aren't
> > enough stats to do its calculations. But other users of this
> > LSNTimeline will simply get highly inaccurate results (I think?). Is
> > there anything we could do about this? It seems bad.
>
> A complication with this over stats is that we can't recompute this in case of
> a crash/corruption issue.  The simple solution is to consider this unlogged
> data and start fresh at every unclean shutdown, but I have a feeling that 
> won't
> be good enough for basing heuristics on?

Yes, I still haven't dealt with this yet. Tomas had a list of
suggestions in an upthread email, so I will spend some time thinking
about those next.

It seems like we might be able to come up with some way of calculating
a valid "default" value or "best guess" which could be used whenever
there isn't enough data. Though, if we crash and lose some time stream
data, we won't know that that data was lost due to a crash so we
wouldn't know to use our "best guess" to make up for it. So, maybe I
should try and rebuild the stream using some combination of WAL, clog,
and commit timestamps? Or perhaps I should do some basic WAL logging
just for this data structure.

> > Andres had brought up something at some point about, what if the
> > database is simply turned off for awhile and then turned back on. Even
> > if you cleanly shut down, will there be "gaps" in the timeline? I
> > think that could be okay, but it is something to think about.
>
> The gaps would represent reality, so there is nothing wrong per se with gaps,
> but if they inflate the interval of a bucket which in turns impact the
> precision of the results then that can be a problem.

Yes, actually I added some hacky code to the quick and dirty python
simulator I wrote [2] to test out having a big gap with no updates (if
there is no db activity so nothing for bgwriter to do or the db is off
for a while). And it seemed to basically work fine.

> While the bucketing algorithm is a clever algorithm for degrading precision 
> for
> older entries without discarding them, I do fear that we'll risk ending up 
> with
> answers like "somewhere between in the past and even further in the past".
> I've been playing around with various compression algorithms for packing the
> buckets such that we can retain precision for longer.  Since you were aiming 
> to
> work on other patches leading up to the freeze, let's pick this up again once
> things calm down.

Let me know what you think about the new algorithm. I wonder if for
points older than the second to oldest point, we have the function
return something like "older than a year ago" instead of guessing. The
new method doesn't focus on compressing old data though.

> When compiling I got this warning for lsntime_merge_target:
>
> pgstat_wal.c:242:1: warning: non-void function does not return a value in all 
> control paths [-Wreturn-type]
> }
> ^
> 1 warning generated.
>
> The issue seems to be that the can't-really-happen path is protected with an
> Assertion, which is a no-op for production builds.  I think we should handle
> the error rather than rely on testing catching it (since if it does happen 
> even
> though it can't, it's going to be when it's for sure not tested).  Returning 
> an
> invalid array subscript like -1 and testing for it in lsntime_insert, with an
> elog(WARNING..), seems enough.
>
>
> -last_snapshot_lsn <= GetLastImportantRecPtr())
> +last_snapshot_lsn <= current_lsn)
> I think we should delay extracting the LSN with GetLastImpo

Re: Add LSN <-> time conversion functionality

[Melanie Plageman]

Thanks so much Bharath, Andrey, and Ilya for the review!

I've posted a new version here [1] which addresses some of your
concerns. I'll comment on those it does not address inline.

On Thu, May 30, 2024 at 1:26 PM Andrey M. Borodin  wrote:
>
> === Questions ===
> 1. The patch does not handle server restart. All pages will need freeze after 
> any crash?

I haven't fixed this yet. See my email for some thoughts on what I
should do here.

> 2. Some benchmarks to proof the patch does not have CPU footprint.

This is still a todo. Typically when designing a benchmark like this,
I would want to pick a worst-case workload to see how bad it could be.
I wonder if just a write heavy workload like pgbench builtin tpcb-like
would be sufficient?

> === Nits ===
> "Timeline" term is already taken.

I changed it to LSNTimeStream. What do you think?

> The patch needs rebase due to some header changes.

I did this.

> Tests fail on Windows.

I think this was because of the compiler warnings, but I need to
double-check now.

> The patch lacks tests.

I thought about this a bit. I wonder what kind of tests make sense.

I could
1) Add tests with the existing stats tests
(src/test/regress/sql/stats.sql) and just test that bgwriter is in
fact adding to the time stream.

2) Or should I add some infrastructure to be able to create an
LSNTimeStream and then insert values to it and do some validations of
what is added? I did a version of this but it is just much more
annoying with C & SQL than with python (where I tried out my
algorithms) [2].

> Some docs would be nice, but the feature is for developers.

I added some docs.

> Mapping is protected for multithreaded access by walstats LWlock and might 
> have tuplestore_putvalues() under that lock. That might be a little 
> dangerous, if tuplestore will be on-disk for some reason (should not happen).

Ah, great point! I forgot about the *fetch_stat*() functions. I used
pgstat_fetch_stat_wal() in the latest version so I have a local copy
that I can stuff into the tuplestore without any locking. It won't be
as up-to-date, but I think that is 100% okay for this function.

- Melanie

[1] 
https://www.postgresql.org/message-id/CAAKRu_a6WSkWPtJCw%3DW%2BP%2Bg-Fw9kfA_t8sMx99dWpMiGHCqJNA%40mail.gmail.com
[2] https://gist.github.com/melanieplageman/95126993bcb43d4b4042099e9d0ccc11

Re: long-standing data loss bug in initial sync of logical replication

[Amit Kapila]

On Wed, Jun 26, 2024 at 4:57 PM Tomas Vondra
 wrote:
>
> On 6/25/24 07:04, Amit Kapila wrote:
> > On Mon, Jun 24, 2024 at 8:06 PM Tomas Vondra
> >  wrote:
> >>
> >> On 6/24/24 12:54, Amit Kapila wrote:
> >>> ...
> 
> >> I'm not sure there are any cases where using SRE instead of AE would 
> >> cause
> >> problems for logical decoding, but it seems very hard to prove. I'd be 
> >> very
> >> surprised if just using SRE would not lead to corrupted cache contents 
> >> in some
> >> situations. The cases where a lower lock level is ok are ones where we 
> >> just
> >> don't care that the cache is coherent in that moment.
> 
> > Are you saying it might break cases that are not corrupted now? How
> > could obtaining a stronger lock have such effect?
> 
>  No, I mean that I don't know if using SRE instead of AE would have 
>  negative
>  consequences for logical decoding. I.e. whether, from a logical decoding 
>  POV,
>  it'd suffice to increase the lock level to just SRE instead of AE.
> 
>  Since I don't see how it'd be correct otherwise, it's kind of a moot 
>  question.
> 
> >>>
> >>> We lost track of this thread and the bug is still open. IIUC, the
> >>> conclusion is to use SRE in OpenTableList() to fix the reported issue.
> >>> Andres, Tomas, please let me know if my understanding is wrong,
> >>> otherwise, let's proceed and fix this issue.
> >>>
> >>
> >> It's in the commitfest [https://commitfest.postgresql.org/48/4766/] so I
> >> don't think we 'lost track' of it, but it's true we haven't done much
> >> progress recently.
> >>
> >
> > Okay, thanks for pointing to the CF entry. Would you like to take care
> > of this? Are you seeing anything more than the simple fix to use SRE
> > in OpenTableList()?
> >
>
> I did not find a simpler fix than adding the SRE, and I think pretty
> much any other fix is guaranteed to be more complex. I don't remember
> all the details without relearning all the details, but IIRC the main
> challenge for me was to convince myself it's a sufficient and reliable
> fix (and not working simply by chance).
>
> I won't have time to look into this anytime soon, so feel free to take
> care of this and push the fix.
>

Okay, I'll take care of this.

-- 
With Regards,
Amit Kapila.

Re: Document NULL

[Yugo NAGATA]

On Tue, 18 Jun 2024 23:02:14 -0700
"David G. Johnston"  wrote:

> On Tuesday, June 18, 2024, Tom Lane  wrote:
> 
> > Yugo NAGATA  writes:
> > > On Tue, 18 Jun 2024 20:56:58 -0700
> > > "David G. Johnston"  wrote:
> > >> But it is neither a keyword nor an identifier.
> >
> > The lexer would be quite surprised by your claim that NULL isn't
> > a keyword.  Per src/include/parser/kwlist.h, NULL is a keyword,
> > and a fully reserved one at that.
> >
> >
> >
> 
> Can’t it be both a value and a keyword?  I figured the not null constraint
> and is null predicates are why it’s a keyword but the existence of those
> doesn’t cover its usage as a literal value that can be stuck anywhere you
> have an expression.

I still wonder it whould be unnecessary to mention the case-insensitivity here
if we can say NULL is *also* a keyword.

Regards,
Yugo Nagata


> David J.


-- 
Yugo NAGATA 

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

[Yugo NAGATA]

On Mon, 24 Jun 2024 08:37:26 -0300
Ranier Vilela  wrote:

> Em seg., 24 de jun. de 2024 às 00:27, Yugo NAGATA 
> escreveu:
> 
> > On Sun, 23 Jun 2024 22:34:03 -0300
> > Ranier Vilela  wrote:
> >
> > > Em dom., 23 de jun. de 2024 às 22:14, Ranier Vilela  > >
> > > escreveu:
> > >
> > > > Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela <
> > ranier...@gmail.com>
> > > > escreveu:
> > > >
> > > >> Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier <
> > > >> mich...@paquier.xyz> escreveu:
> > > >>
> > > >>> On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote:
> > > >>> > It's not critical code, so I think it's ok to use strlen, even
> > because
> > > >>> the
> > > >>> > result of strlen will already be available using modern compilers.
> > > >>> >
> > > >>> > So, I think it's ok to use memcpy with strlen + 1.
> > > >>>
> > > >>> It seems to me that there is a pretty good argument to just use
> > > >>> strlcpy() for the same reason as the one you cite: this is not a
> > > >>> performance-critical code, and that's just safer.
> > > >>>
> > > >> Yeah, I'm fine with strlcpy. I'm not against it.
> > > >>
> > > > Perhaps, like the v2?
> > > >
> > > > Either v1 or v2, to me, looks good.
> > > >
> > > Thinking about, does not make sense the field size MAXPGPATH + 1.
> > > all other similar fields are just MAXPGPATH.
> > >
> > > If we copy MAXPGPATH + 1, it will also be wrong.
> > > So it is necessary to adjust logbackup.h as well.
> >
> > I am not sure whether we need to change the size of the field,
> > but if change it, I wonder it is better to modify the following
> > message from MAXPGPATH to MAXPGPATH -1.
> >
> >  errmsg("backup label too long (max %d
> > bytes)",
> > MAXPGPATH)));
> >
> Or perhaps, is it better to show the too long label?
> 
> errmsg("backup label too long (%s)",
> backupidstr)));

I don't think it is better to show a string longer than MAXPGPATH (=1024)
in the error message.

Regards,
Yugo Nagata

> 
> best regards,
> Ranier Vilela
> 
> >
> > >
> > > So, I think that v3 is ok to fix.
> > >
> > > best regards,
> > > Ranier Vilela
> > >
> > > >
> > > > best regards,
> > > > Ranier Vilela
> > > >
> > > >>
> >
> >
> > --
> > Yugo NAGATA 
> >


-- 
Yugo NAGATA 

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

[Yugo NAGATA]

On Mon, 24 Jun 2024 08:25:36 -0300
Ranier Vilela  wrote:

> Em dom., 23 de jun. de 2024 às 23:56, Richard Guo 
> escreveu:
> 
> > On Mon, Jun 24, 2024 at 7:51 AM Ranier Vilela  wrote:
> > > In src/include/access/xlogbackup.h, the field *name*
> > > has one byte extra to store null-termination.
> > >
> > > But, in the function *do_pg_backup_start*,
> > > I think that is a mistake in the line (8736):
> > >
> > > memcpy(state->name, backupidstr, strlen(backupidstr));
> > >
> > > memcpy with strlen does not copy the whole string.
> > > strlen returns the exact length of the string, without
> > > the null-termination.
> >
> > I noticed that the two callers of do_pg_backup_start both allocate
> > BackupState with palloc0.  Can we rely on this to ensure that the
> > BackupState.name is initialized with null-termination?
> >
> I do not think so.
> It seems to me the best solution is to use Michael's suggestion, strlcpy +
> sizeof.
> 
> Currently we have this:
> memcpy(state->name, "longlongpathexample1",
> strlen("longlongpathexample1"));
> printf("%s\n", state->name);
> longlongpathexample1
> 
> Next random call:
> memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
> printf("%s\n", state->name);
> longpathexample2ple1

In the current uses, BackupState is freed (by pfree or MemoryContextDelete)
after each use of BackupState, so the memory space is not reused as your
scenario above, and there would not harms even if the null-termination is 
omitted. 

However, I wonder it is better to use strlcpy without assuming such the good
manner of callers.

Regards,
Yugo Nagata

> 
> It's not a good idea to use memcpy with strlen.
> 
> best regards,
> Ranier Vilela


-- 
Yugo NAGATA 

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

[Michael Paquier]

On Thu, Jun 27, 2024 at 12:17:04PM +0900, Yugo NAGATA wrote:
> I don't think it is better to show a string longer than MAXPGPATH (=1024)
> in the error message.

+1.  I am not convinced that this is useful in this context.
--
Michael


signature.asc
Description: PGP signature

Re: thread-safety: gmtime_r(), localtime_r()

[Stepan Neretin]

On Thu, Jun 27, 2024 at 1:42 AM Peter Eisentraut 
wrote:

> Here is a patch for using gmtime_r() and localtime_r() instead of
> gmtime() and localtime(), for thread-safety.
>
> There are a few affected calls in libpq and ecpg's libpgtypes, which are
> probably effectively bugs, because those libraries already claim to be
> thread-safe.
>
> There is one affected call in the backend.  Most of the backend
> otherwise uses the custom functions pg_gmtime() and pg_localtime(),
> which are implemented differently.
>
> Some portability fun: gmtime_r() and localtime_r() are in POSIX but are
> not available on Windows.  Windows has functions gmtime_s() and
> localtime_s() that can fulfill the same purpose, so we can add some
> small wrappers around them.  (Note that these *_s() functions are also
> different from the *_s() functions in the bounds-checking extension of
> C11.  We are not using those here.)
>
> MinGW exposes neither *_r() nor *_s() by default.  You can get at the
> POSIX-style *_r() functions by defining _POSIX_C_SOURCE appropriately
> before including .  (There is apparently probably also a way to
> get at the Windows-style *_s() functions by supplying some additional
> options or defines.  But we might as well just use the POSIX ones.)
>
>
Hi! Looks good to me.
But why you don`t change localtime function at all places?
For example:
src/bin/pg_controldata/pg_controldata.c
src/bin/pg_dump/pg_backup_archiver.c
src/bin/initdb/findtimezone.c
Best regards, Stepan Neretin.

Re: Conflict Detection and Resolution

[shveta malik]

On Wed, Jun 26, 2024 at 2:33 PM Amit Kapila  wrote:
>
> On Tue, Jun 25, 2024 at 3:39 PM shveta malik  wrote:
> >
> > On Tue, Jun 25, 2024 at 3:12 PM Amit Kapila  wrote:
> > >
> > > On Mon, Jun 24, 2024 at 1:47 PM shveta malik  
> > > wrote:
> > > >
> > > > On Thu, Jun 20, 2024 at 6:41 PM Amit Kapila  
> > > > wrote:
> > > > >
> > > > > >> In the second patch, we can implement simple built-in resolution
> > > > > >> strategies like apply and skip (which can be named as remote_apply 
> > > > > >> and
> > > > > >> keep_local, see [3][4] for details on these strategies) with ERROR 
> > > > > >> or
> > > > > >> LOG being the default strategy. We can allow these strategies to be
> > > > > >> configured at the global and table level.
> > > >
> > > > Before we implement resolvers, we need a way to configure them. Please
> > > > find the patch002 which attempts to implement Global Level Conflict
> > > > Resolvers Configuration.  Note that patch002 is dependent upon
> > > > Conflict-Detection patch001 which is reviewed in another thread [1].
> > > > I have attached patch001 here for convenience and to avoid CFBot
> > > > failures. But please use [1] if you have any comments on patch001.
> > > >
> > > > New DDL commands in patch002 are:
> > > >
> > > > To set global resolver for given conflcit_type:
> > > > SET CONFLICT RESOLVER 'conflict_resolver' FOR 'conflict_type'
> > > >
> > > > To reset to default resolver:
> > > > RESET CONFLICT RESOLVER FOR 'conflict_type'
> > > >
> > >
> > > Does setting up resolvers have any meaning without subscriptions? I am
> > > wondering whether we should allow to set up the resolvers at the
> > > subscription level. One benefit is that users don't need to use a
> > > different DDL to set up resolvers. The first patch gives a conflict
> > > detection option at the subscription level, so it would be symmetrical
> > > to provide a resolver at the subscription level. Yet another benefit
> > > could be that it provides users facility to configure different
> > > resolvers for a set of tables belonging to a particular
> > > publication/node.
> >
> > There can be multiple tables included in a publication with varying
> > business use-cases and thus may need different resolvers set, even
> > though they all are part of the same publication.
> >
>
> Agreed but this is the reason we are planning to keep resolvers at the
> table level. Here, I am asking to set resolvers at the subscription
> level rather than at the global level.

Okay, got it. I misunderstood earlier that we want to replace table
level resolvers with subscription ones.
Having global configuration has one benefit that if the user has no
requirement to set different resolvers for different subscriptions or
tables, he may always set one global configuration and be done with
it. OTOH, I also agree with benefits coming with subscription level
configuration.

thanks
Shveta

Re: Patch bug: Fix jsonpath .* on Arrays

[Stepan Neretin]

On Thu, Jun 27, 2024 at 1:16 AM David E. Wheeler 
wrote:

> On Jun 25, 2024, at 13:48, David E. Wheeler  wrote:
>
> > I have since realized it’s not a complete fix for the issue, and hacked
> around it in my Go version. Would be fine to remove that bit, but IIRC this
> was the only execution function that would return `jperNotFound` when it in
> fact adds items to the `found` list. The current implementation only looks
> at one or the other, so it’s not super important, but I found the
> inconsistency annoying and sometimes confusing.
>
> I’ve removed this change.
>
> >> [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
> >> I propose adding a similar test with explicitly specified lax mode:
> select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what
> lax mode is set by default.
> >
> > Very few of the other tests do so; I can add it if it’s important for
> this case, though.
>
> Went ahead and added lax.
>
> > @? suppresses a number of errors. Perhaps I should add a variant of the
> error-raising query that passes the silent arg, which would also suppress
> the error.
>
> Added a variant where the silent param suppresses the error, too.
>
> V2 attached and the PR updated:
>
>   https://github.com/theory/postgres/pull/4/files
>
> Best,
>
> David
>
>
>
>
HI! Now it looks good for me.
Best regards, Stepan Neretin.

Re: Track the amount of time waiting due to cost_delay

[Masahiko Sawada]

On Mon, Jun 24, 2024 at 7:50 PM Bertrand Drouvot
 wrote:
>
> Hi,
>
> On Sat, Jun 22, 2024 at 12:48:33PM +, Bertrand Drouvot wrote:
> > 1. vacuuming indexes time has been longer on master because with v2, the 
> > leader
> > has been interrupted 342605 times while waiting, then making v2 "faster".
> >
> > 2. the leader being interrupted while waiting is also already happening on 
> > master
> > due to the pgstat_progress_parallel_incr_param() calls in
> > parallel_vacuum_process_one_index() (that have been added in
> > 46ebdfe164). It has been the case "only" 36 times during my test case.
> >
> > I think that 2. is less of a concern but I think that 1. is something that 
> > needs
> > to be addressed because the leader process is not honouring its cost delay 
> > wait
> > time in a noticeable way (at least during my test case).
> >
> > I did not think of a proposal yet, just sharing my investigation as to why
> > v2 has been faster than master during the vacuuming indexes phase.

Thank you for the benchmarking and analyzing the results! I agree with
your analysis and was surprised by the fact that the more times
workers go to sleep, the more times the leader wakes up.

>
> I think that a reasonable approach is to make the reporting from the parallel
> workers to the leader less aggressive (means occur less frequently).
>
> Please find attached v3, that:
>
> - ensures that there is at least 1 second between 2 reports, per parallel 
> worker,
> to the leader.
>
> - ensures that the reported delayed time is still correct (keep track of the
> delayed time between 2 reports).
>
> - does not add any extra pg_clock_gettime_ns() calls (as compare to v2).
>

Sounds good to me. I think it's better to keep the logic for
throttling the reporting the delay message simple. It's an important
consideration but executing parallel vacuum with delays would be less
likely to be used in practice.

Regards,

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

Adminpack removal

[Philippe BEAUDOIN]

Hi,

I have just tested PG17 beta1 with the E-Maj solution I maintain. The 
only issue I found is the removal of the adminpack contrib.


In the emaj extension, which is the heart of the solution, and which is 
written in plpgsql, the code just uses the pg_file_unlink() function to 
automatically remove files produced by COPY TO statements when no rows 
have been written. In some specific use cases, it avoids the user to get 
a few interesting files among numerous empty files in a directory. I 
have found a workaround. That's a little bit ugly, but it works. So this 
is not blocking for me.


FYI, the project's repo is on github (https://github.com/dalibo/emaj), 
which was supposed to be scanned to detect potential adminpack usages.


Finally, I wouldn't be surprise if some other user projects or 
applications use adminpack as this is a simple way to get sql functions 
that write, rename or remove files.


Regards.

Re: libpq: Fix lots of discrepancies in PQtrace

[Michael Paquier]

On Wed, Jun 26, 2024 at 10:02:08PM +0200, Jelte Fennema-Nio wrote:
> On Wed, 26 Jun 2024 at 18:36, Alvaro Herrera  wrote:
> > Thanks, Nathan.  I'm holding myself responsible for the rest ... will
> > handle soon after the branch.
> 
> Sounds great. Out of curiosity, what is the backpatching policy for
> something like this? Honestly most of these patches could be
> considered bugfixes in PQtrace, so backpatching might make sense. OTOH
> I don't think PQtrace is used very much in practice, so backpatching
> might carry more risk than it's worth.

0001 getting on HEAD after the feature freeze as a cleanup piece
cleanup is no big deal.  That's cosmetic, still OK.

Looking at the whole, the rest of the patch set qualifies as a new
feature, even if they're aimed at closing existing gaps.
Particularly, you have bits of new infrastructure introduced in libpq
like the current_auth_response business in 0004, making it a new
feature by structure.

+   conn->current_auth_response = AUTH_RESP_PASSWORD;
ret = pqPacketSend(conn, PqMsg_PasswordMessage, pwd_to_send, 
strlen(pwd_to_send) + 1);
+   conn->current_auth_response = AUTH_RESP_NONE;

It's a surprising approach.  Future callers of pqPacketSend() and
pqPutMsgEnd() would easily miss that this flag should be set, as much
as reset.  Isn't that something that should be added in input of these
functions?

+   AuthResponseType current_auth_response;
I'd recommend to document what this flag is here for, with a comment.
--
Michael


signature.asc
Description: PGP signature

Re: walsender.c comment with no context is hard to understand

[Michael Paquier]

On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
> On Mon, 3 Jun 2024 at 11:21, Peter Smith  wrote:
>> Perhaps the comment should say something like it used to:
>> /* Fail if there is not enough WAL available. This can happen during
>> shutdown. */
> 
> Agree with this, +1 for this change.

That would be an improvement.  Would you like to send a patch with all
the areas you think could stand for improvements?
--
Michael


signature.asc
Description: PGP signature

Re: thread-safety: gmtime_r(), localtime_r()

[Peter Eisentraut]

On 27.06.24 06:47, Stepan Neretin wrote:

Hi! Looks good to me.
But why you don`t change localtime function at all places?
For example:
src/bin/pg_controldata/pg_controldata.c
src/bin/pg_dump/pg_backup_archiver.c
src/bin/initdb/findtimezone.c


At the moment, I am focusing on the components that are already meant to 
be thread-safe (libpq, ecpg libs) and the ones we are actively looking 
at maybe converting (backend).  I don't intend at this point to convert 
all other code to use only thread-safe APIs.

Re: Adminpack removal

[Hannu Krosing]

I agree that removing adminpack was a bit of a surprise for me as
well. At first I assumed that it was just moved into the core to
accompany the file and directory *reading* functions, until I found
the release notes mentioning that now one of the users of adminpack
does not need it and so it is dropped.

The easy and currently supported-in-core way to do file manipulation
is using pl/pythonu or pl/perlu but I agree that it is an overkill if
all you need is a little bit of file manipulation.

Best Regards
Hannu

On Thu, Jun 27, 2024 at 7:34 AM Philippe BEAUDOIN  wrote:
>
> Hi,
>
> I have just tested PG17 beta1 with the E-Maj solution I maintain. The
> only issue I found is the removal of the adminpack contrib.
>
> In the emaj extension, which is the heart of the solution, and which is
> written in plpgsql, the code just uses the pg_file_unlink() function to
> automatically remove files produced by COPY TO statements when no rows
> have been written. In some specific use cases, it avoids the user to get
> a few interesting files among numerous empty files in a directory. I
> have found a workaround. That's a little bit ugly, but it works. So this
> is not blocking for me.
>
> FYI, the project's repo is on github (https://github.com/dalibo/emaj),
> which was supposed to be scanned to detect potential adminpack usages.
>
> Finally, I wouldn't be surprise if some other user projects or
> applications use adminpack as this is a simple way to get sql functions
> that write, rename or remove files.
>
> Regards.
>
>
>

RE: pg_createsubscriber: drop pre-existing subscriptions from the converted node

[Hayato Kuroda (Fujitsu)]

Dear Amit, Vingesh,

Thanks for giving comments!

> It seems disabling subscriptions on the primary can make the primary
> stop functioning for some duration of time. I feel we need some
> solution where after converting to subscriber, we disable and drop
> pre-existing subscriptions. One idea could be that we use the list of
> new subscriptions created by the tool such that any subscription not
> existing in that list will be dropped.

Previously I avoided coding like yours, because there is a room that converted
node can connect to another publisher. But per off-list discussion, we can skip
it by setting max_logical_replication_workers = 0. I refactored with the 
approach.
Note that the GUC is checked at verification phase, so an attribute is added to
start_standby_server() to select the workload.

Most of comments by Vignesh were invalidated due to the code change, but I hoped
I checked your comments were not reproduced. Also, 0001 was created to remove an
unused attribute.

> Shouldn't this be an open item for PG17?

Added this thread to wikipage.

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


v2-0001-pg_createsubscriber-remove-unused-attribute.patch
Description: v2-0001-pg_createsubscriber-remove-unused-attribute.patch


v2-0002-pg_createsubscriber-Drop-pre-existing-subscriptio.patch
Description:  v2-0002-pg_createsubscriber-Drop-pre-existing-subscriptio.patch