Re: cfbot is listing committed patches?

[Peter Smith]

On Tue, Apr 11, 2023 at 4:36 PM Thomas Munro  wrote:
>
> On Tue, Apr 11, 2023 at 6:16 PM Peter Smith  wrote:
> > cfbot [1] is listing some already committed patches under the "Needs
> > Review" category. For example here are some of mine [1][2]. And
> > because they are already committed, the 'apply'  fails, so they get
> > flagged by cfbot as needed rebase.
> >
> > Something seems broken.
>
> Thanks, fixed.  It was confused because after CF #42 was recently
> closed, #43 became "current" but there is not yet a "next" commitfest.
> I never noticed before, but I guess those are manually created.

Oh, right. And I was mistakenly looking at the cfbot page of the
"next" commitfest even though there is no such thing. Thanks for the
fix/explanation.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: When to drop src/tools/msvc support

[Magnus Hagander]

On Tue, Apr 11, 2023 at 12:27 AM Andres Freund  wrote:
>
> Hi,
>
> On 2023-04-10 19:55:35 +0100, Dave Page wrote:
> > Projects other than the EDB installers use the MSVC build system - e.g.
> > pgAdmin uses it’s own builds of libpq and other tools (psql, pg_dump etc)
> > that are pretty heavily baked into a fully automated build system (even the
> > build servers and all their requirements are baked into Ansible).
> >
> > Changing that lot would be non-trivial, though certainly possible, and I
> > suspect we’re not the only ones doing that sort of thing.
>
> Do you have a link to the code for that, if it's open? Just to get an
> impression for how hard it'd be to switch over?


The pgadmin docs/readme refers to
https://github.com/pgadmin-org/pgadmin4/tree/master/pkg/win32

It clearly doesn't have the full automation stuff, but appears to have
the parts about building the postgres dependency.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

A minor adjustment to get_cheapest_path_for_pathkeys

[Richard Guo]

The check for parallel_safe should be even cheaper than cost comparison
so I think it's better to do that first.  The attached patch does this
and also updates the comment to mention the requirement about being
parallel-safe.

Thanks
Richard


v1-0001-Adjustment-to-get_cheapest_path_for_pathkeys.patch
Description: Binary data

Re: Improve logging when using Huge Pages

[Kyotaro Horiguchi]

At Tue, 11 Apr 2023 15:17:46 +0900, Michael Paquier  wrote 
in 
> On Thu, Mar 23, 2023 at 05:25:46PM +0900, Kyotaro Horiguchi wrote:
> > Wouldn't storing the value in the shared memory itself work? Though,
> > that space does become almost dead for the server's lifetime...
> 
> Sure, it would work.  However, we'd still need an interface for the
> extra function.  At this point, a GUC with an unknown state is kind of
> OK for me.  Anyway, where would you stick this state?

(Digging memory..)

Sorry for confusion but I didn't mean to stick to the function.  Just
I thought that some people seem to dislike having the third state for
the should-be-boolean variable.

So, I'm okay with GUC, having "unknown".

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Peter Smith]

Here are a few more review comments for patch v3-0001.

==
doc/src/sgml/ref/pgupgrade.sgml

1.
+ 
+  --include-logical-replication-slots
+  
+   
+Upgrade logical replication slots. Only permanent replication slots
+included. Note that pg_upgrade does not check the installation of
+plugins.
+   
+  
+ 

Missing word.

"Only permanent replication slots included." --> "Only permanent
replication slots are included."

==
src/bin/pg_dump/pg_dump.c

2. help

@@ -1119,6 +1145,8 @@ help(const char *progname)
  printf(_("  --no-unlogged-table-data do not dump unlogged table data\n"));
  printf(_("  --on-conflict-do-nothing add ON CONFLICT DO NOTHING
to INSERT commands\n"));
  printf(_("  --quote-all-identifiers  quote all identifiers, even
if not key words\n"));
+ printf(_("  --logical-replication-slots-only\n"
+ "   dump only logical replication slots,
no schema or data\n"));
  printf(_("  --rows-per-insert=NROWS  number of rows per INSERT;
implies --inserts\n"));
A previous review comment ([1] #11b) seems to have been missed. This
help is misplaced. It should be in alphabetical order consistent with
all the other help.

==
src/bin/pg_dump/pg_dump.h

3. _LogicalReplicationSlotInfo

+/*
+ * The LogicalReplicationSlotInfo struct is used to represent replication
+ * slots.
+ * XXX: add more attrbutes if needed
+ */
+typedef struct _LogicalReplicationSlotInfo
+{
+ DumpableObject dobj;
+ char*plugin;
+ char*slottype;
+ char*twophase;
+} LogicalReplicationSlotInfo;
+

4a.
The indent of the 'LogicalReplicationSlotInfo' looks a bit strange,
unlike others in this file. Is it OK?

~

4b.
There was no typedefs.list file in this patch. Maybe the above
whitespace problem is a result of that omission.

==
.../pg_upgrade/t/003_logical_replication.pl

5.

+# Run pg_upgrade. pg_upgrade_output.d is removed at the end
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $old_publisher->data_dir,
+ '-D', $new_publisher->data_dir,
+ '-b', $bindir,
+ '-B', $bindir,
+ '-s', $new_publisher->host,
+ '-p', $old_publisher->port,
+ '-P', $new_publisher->port,
+ $mode,'--include-logical-replication-slot'
+ ],
+ 'run of pg_upgrade for new publisher');

5a.
How can this test even be working as-expected with those options?

Here it is passing option '--include-logical-replication-slot' but
AFAIK the proper option name everywhere else in this patch is
'--include-logical-replication-slots' (with the 's')

~

5b.
I'm not sure that "pg_upgrade for new publisher" makes sense.

It's more like "pg_upgrade of old publisher", or simply "pg_upgrade of
publisher"

--
[1] 
https://www.postgresql.org/message-id/TYCPR01MB5870E212F5012FD6272CE1E3F5969%40TYCPR01MB5870.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia

ERROR messages in VACUUM's PARALLEL option

[David Rowley]

Over in [1], Horiguchisan mentioned a few things about VACUUM's new
BUFFER_USAGE_LIMIT option.

1) buffer_usage_limit in the ERROR messages should be consistently in uppercase.
2) defGetString() already checks for opt->args == NULL and raises an
ERROR when it is.

I suspect that Melanie might have followed the lead of the PARALLEL
option when she was working on adding the BUFFER_USAGE_LIMIT patch.

What I'm wondering now is:

a) Is it worth changing this for the PARALLEL option too? and;
b) I see we lose the parse position indicator, and;
c) If we did want to change this, is it too late for v16?

For a), I know that's much older code, so perhaps it's not worth
messing around with the ERROR messages for this. For b), this seems
like a fairly minor detail given that VACUUM commands are fairly
simple. It shouldn't be too hard for a user to see what we're talking
about.

I've attached a patch to adjust this.

David

[1] 
https://postgr.es/m/20230411.102335.1643720544536884844.horikyota@gmail.com


vacuum_parallel_errors.patch
Description: Binary data

Re: When to drop src/tools/msvc support

[Dave Page]

On Tue, 11 Apr 2023 at 08:09, Magnus Hagander  wrote:

> On Tue, Apr 11, 2023 at 12:27 AM Andres Freund  wrote:
> >
> > Hi,
> >
> > On 2023-04-10 19:55:35 +0100, Dave Page wrote:
> > > Projects other than the EDB installers use the MSVC build system - e.g.
> > > pgAdmin uses it’s own builds of libpq and other tools (psql, pg_dump
> etc)
> > > that are pretty heavily baked into a fully automated build system
> (even the
> > > build servers and all their requirements are baked into Ansible).
> > >
> > > Changing that lot would be non-trivial, though certainly possible, and
> I
> > > suspect we’re not the only ones doing that sort of thing.
> >
> > Do you have a link to the code for that, if it's open? Just to get an
> > impression for how hard it'd be to switch over?
>
>
> The pgadmin docs/readme refers to
> https://github.com/pgadmin-org/pgadmin4/tree/master/pkg/win32
>
> It clearly doesn't have the full automation stuff, but appears to have
> the parts about building the postgres dependency.
>

Yeah, that's essentially the manual process, though I haven't tested it in
a while. The Ansible stuff is not currently public. I suspect (or rather,
hope) that we can pull in all the additional packages required using
Chocolatey which shouldn't be too onerous.

Probably my main concern is that the Meson build can use the same version
of the VC++ compiler that we use (v14), which is carefully matched for
compatibility with all the various components, just in case anything passes
CRT pointers around. Python is the one thing we don't build ourselves on
Windows and the process will build modules like gssapi and psycopg (which
links with libpq of course), so we're basically following what they use.

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: Minimal logical decoding on standbys

[Drouvot, Bertrand]

Hi,

On 4/11/23 7:36 AM, Noah Misch wrote:

On Fri, Apr 07, 2023 at 11:12:26AM -0700, Andres Freund wrote:

--- /dev/null
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -0,0 +1,720 @@
+# logical decoding on standby : test logical decoding,
+# recovery conflict and standby promotion.

...

+$node_primary->append_conf('postgresql.conf', q{
+wal_level = 'logical'
+max_replication_slots = 4
+max_wal_senders = 4
+log_min_messages = 'debug2'
+log_error_verbosity = verbose
+});


Buildfarm member hoverfly stopped reporting in when this test joined the tree.
It's currently been stuck here for 140 minutes:



Thanks for the report!

It's looping on:

2023-04-11 02:57:52.516 UTC [62718288:5] 035_standby_logical_decoding.pl LOG:  
0: statement: SELECT restart_lsn IS NOT NULL
FROM pg_catalog.pg_replication_slots WHERE slot_name = 
'promotion_inactiveslot'

And the reason is that the slot is not being created:

$ grep "CREATE_REPLICATION_SLOT" 035_standby_logical_decoding_standby.log | 
tail -2
2023-04-11 02:57:47.287 UTC [9241178:15] 035_standby_logical_decoding.pl STATEMENT:  
CREATE_REPLICATION_SLOT "otherslot" LOGICAL "test_decoding" ( SNAPSHOT 
'nothing')
2023-04-11 02:57:47.622 UTC [9241178:23] 035_standby_logical_decoding.pl STATEMENT:  
CREATE_REPLICATION_SLOT "otherslot" LOGICAL "test_decoding" ( SNAPSHOT 
'nothing')

Not sure why the slot is not being created.

There is also "replication apply delay" increasing:

2023-04-11 02:57:49.183 UTC [13304488:253] DEBUG:  0: sendtime 2023-04-11 
02:57:49.111363+00 receipttime 2023-04-11 02:57:49.183512+00 replication apply 
delay 644 ms transfer latency 73 ms
2023-04-11 02:57:49.184 UTC [13304488:259] DEBUG:  0: sendtime 2023-04-11 
02:57:49.183461+00 receipttime 2023-04-11 02:57:49.1842+00 replication apply 
delay 645 ms transfer latency 1 ms
2023-04-11 02:57:49.221 UTC [13304488:265] DEBUG:  0: sendtime 2023-04-11 
02:57:49.184166+00 receipttime 2023-04-11 02:57:49.221059+00 replication apply 
delay 682 ms transfer latency 37 ms
2023-04-11 02:57:49.222 UTC [13304488:271] DEBUG:  0: sendtime 2023-04-11 
02:57:49.221003+00 receipttime 2023-04-11 02:57:49.222144+00 replication apply 
delay 683 ms transfer latency 2 ms
2023-04-11 02:57:49.222 UTC [13304488:277] DEBUG:  0: sendtime 2023-04-11 
02:57:49.222095+00 receipttime 2023-04-11 02:57:49.2228+00 replication apply 
delay 684 ms transfer latency 1 ms

Noah, I think hoverfly is yours, would it be possible to have access (I'm not 
an AIX expert though) or check if you see a slot creation hanging and if so why?


===
$ tail -n5 regress_log_035_standby_logical_decoding
[02:57:48.390](0.100s) ok 66 - otherslot on standby not dropped

### Reloading node "standby"
# Running: pg_ctl -D 
/scratch/nm/farm/xlc64v16/HEAD/pgsql.build/src/test/recovery/tmp_check/t_035_standby_logical_decoding_standby_data/pgdata
 reload
server signaled
===

I've posted a tarball of the current logs at
https://drive.google.com/file/d/1JIZ5hSHBsKjEgU5WOGHOqXB7Z_-9XT5u/view?usp=sharing.
The test times out (PG_TEST_TIMEOUT_DEFAULT=5400), and uploading logs then
fails with 413 Request Entity Too Large.  Is the above
log_min_messages='debug2' important?  Removing that may make the logs small
enough to upload normally.


I think debug2 might still be useful while investigating this issue (I'll 
compare a working TAP run with this one).

Regards

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

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Peter Smith]

On Sat, Apr 8, 2023 at 12:00 AM Hayato Kuroda (Fujitsu)
 wrote:
>
...
> > 17. main
> >
> > + /*
> > + * Create replication slots if requested.
> > + *
> > + * XXX This must be done after doing pg_resetwal command because the
> > + * command will remove required WALs.
> > + */
> > + if (user_opts.include_slots)
> > + {
> > + start_postmaster(&new_cluster, true);
> > + create_replicaiton_slots();
> > + stop_postmaster(false);
> > + }
> > +
> >
> > I don't think that warrants a "XXX" style comment. It is just a "Note:".
>
> Fixed. Could you please tell me the classification of them if you can?

Hopefully, someone will correct me if this explanation is wrong, but
my understanding of the different prefixes is like this --

"XXX" is used as a marker for future developers to consider maybe
revisiting/improving something that the comment refers to
e.g.
/* XXX - it would be better to code this using blah but for now we did
not */
/* XXX - option 'foo' is not currently supported but... */
/* XXX - it might be worth considering adding more checks or an assert
here because... */

OTOH, "Note" is just for highlighting why something is the way it is,
but with no implication that it should be revisited/changed in the
future.
e.g.
/* Note: We deliberately do not test the state here because... */
/* Note: This memory must be zeroed because... */
/* Note: This string has no '\0' terminator so... */

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Minimal logical decoding on standbys

[Drouvot, Bertrand]

Hi,

On 4/11/23 10:20 AM, Drouvot, Bertrand wrote:

Hi,

On 4/11/23 7:36 AM, Noah Misch wrote:

On Fri, Apr 07, 2023 at 11:12:26AM -0700, Andres Freund wrote:

--- /dev/null
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -0,0 +1,720 @@
+# logical decoding on standby : test logical decoding,
+# recovery conflict and standby promotion.

...

+$node_primary->append_conf('postgresql.conf', q{
+wal_level = 'logical'
+max_replication_slots = 4
+max_wal_senders = 4
+log_min_messages = 'debug2'
+log_error_verbosity = verbose
+});


Buildfarm member hoverfly stopped reporting in when this test joined the tree.
It's currently been stuck here for 140 minutes:



Thanks for the report!

It's looping on:

2023-04-11 02:57:52.516 UTC [62718288:5] 035_standby_logical_decoding.pl LOG:  
0: statement: SELECT restart_lsn IS NOT NULL
     FROM pg_catalog.pg_replication_slots WHERE slot_name = 
'promotion_inactiveslot'

And the reason is that the slot is not being created:

$ grep "CREATE_REPLICATION_SLOT" 035_standby_logical_decoding_standby.log | 
tail -2
2023-04-11 02:57:47.287 UTC [9241178:15] 035_standby_logical_decoding.pl STATEMENT:  
CREATE_REPLICATION_SLOT "otherslot" LOGICAL "test_decoding" ( SNAPSHOT 
'nothing')
2023-04-11 02:57:47.622 UTC [9241178:23] 035_standby_logical_decoding.pl STATEMENT:  
CREATE_REPLICATION_SLOT "otherslot" LOGICAL "test_decoding" ( SNAPSHOT 
'nothing')

Not sure why the slot is not being created.

There is also "replication apply delay" increasing:

2023-04-11 02:57:49.183 UTC [13304488:253] DEBUG:  0: sendtime 2023-04-11 
02:57:49.111363+00 receipttime 2023-04-11 02:57:49.183512+00 replication apply 
delay 644 ms transfer latency 73 ms
2023-04-11 02:57:49.184 UTC [13304488:259] DEBUG:  0: sendtime 2023-04-11 
02:57:49.183461+00 receipttime 2023-04-11 02:57:49.1842+00 replication apply 
delay 645 ms transfer latency 1 ms
2023-04-11 02:57:49.221 UTC [13304488:265] DEBUG:  0: sendtime 2023-04-11 
02:57:49.184166+00 receipttime 2023-04-11 02:57:49.221059+00 replication apply 
delay 682 ms transfer latency 37 ms
2023-04-11 02:57:49.222 UTC [13304488:271] DEBUG:  0: sendtime 2023-04-11 
02:57:49.221003+00 receipttime 2023-04-11 02:57:49.222144+00 replication apply 
delay 683 ms transfer latency 2 ms
2023-04-11 02:57:49.222 UTC [13304488:277] DEBUG:  0: sendtime 2023-04-11 
02:57:49.222095+00 receipttime 2023-04-11 02:57:49.2228+00 replication apply 
delay 684 ms transfer latency 1 ms

Noah, I think hoverfly is yours, would it be possible to have access (I'm not 
an AIX expert though) or check if you see a slot creation hanging and if so why?



Well, we can see in 035_standby_logical_decoding_standby.log:

2023-04-11 02:57:49.180 UTC [62718258:5] [unknown] FATAL:  3D000: database 
"testdb" does not exist

While, on the primary:

2023-04-11 02:57:48.505 UTC [62718254:5] 035_standby_logical_decoding.pl LOG:  
0: statement: CREATE DATABASE testdb

The TAP test is doing:

"
##
# Test standby promotion and logical decoding behavior
# after the standby gets promoted.
##

$node_standby->reload;

$node_primary->psql('postgres', q[CREATE DATABASE testdb]);
$node_primary->safe_psql('testdb', qq[CREATE TABLE decoding_test(x integer, y 
text);]);

# create the logical slots
create_logical_slots($node_standby, 'promotion_');
"

I think we might want to add:

$node_primary->wait_for_replay_catchup($node_standby);

before calling the slot creation.

It's done in the attached, would it be possible to give it a try please?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comdiff --git a/src/test/recovery/t/035_standby_logical_decoding.pl 
b/src/test/recovery/t/035_standby_logical_decoding.pl
index ba98a18bd2..ad845aee28 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -653,6 +653,9 @@ $node_standby->reload;
 $node_primary->psql('postgres', q[CREATE DATABASE testdb]);
 $node_primary->safe_psql('testdb', qq[CREATE TABLE decoding_test(x integer, y 
text);]);
 
+# Wait for the standby to catchup before creating the slots
+$node_primary->wait_for_replay_catchup($node_standby);
+
 # create the logical slots
 create_logical_slots($node_standby, 'promotion_');
 

Re: Support logical replication of DDLs

[Amit Kapila]

On Mon, Apr 10, 2023 at 3:16 PM Zhijie Hou (Fujitsu)
 wrote:
>
> On Friday, April 7, 2023 11:23 amhouzj.f...@fujitsu.com 
>  wrote:
> >
> > On Friday, April 7, 2023 11:13 AM houzj.f...@fujitsu.com
> > 
> > >
> > > On Tuesday, April 4, 2023 7:35 PM shveta malik
> > > 
> > > wrote:
> > > >
> > > > On Tue, Apr 4, 2023 at 8:43 AM houzj.f...@fujitsu.com
> > > >  wrote:
> > > >
> > > > > Attach the new version patch set which did the following changes:
> > > > >
> > > >
> > > > Hello,
> > > >
> > > > I tried below:
> > > > pubnew=# ALTER PUBLICATION mypub2 SET (ddl = 'table'); ALTER
> > > > PUBLICATION
> > > >
> > > > pubnew=# \dRp+
> > > > Publication mypub2 Owner  |
> > > > All tables
> > > > | All DDLs | Table DDLs |
> > > > ++--++-
> > > > shveta | t  | f   | f
> > > > (1 row)
> > > >
> > > > I still see 'Table DDLs' as false and ddl replication did not work for 
> > > > this case.
> > >
> > > Thanks for reporting.
> > >
> > > Attach the new version patch which include the following changes:
> > > * Fix the above bug for ALTER PUBLICATION SET.
> > > * Modify the corresponding event trigger when user execute ALTER
> > > PUBLICATION SET to change the ddl option.
> > > * Fix a miss in pg_dump's code which causes CFbot failure.
> > > * Rebase the patch due to recent commit 4826759.
>
> Another thing I find might need to be improved is about the pg_dump handling 
> of
> the built-in event trigger. Currently, we skip dumping the event trigger which
> are used for ddl replication based on the trigger 
> names(pg_deparse_trig_%s_%u),
> because they will be created along with create publication command. Referring
> to other built-in triggers(foreign key trigger), it has a tgisinternal catalog
> column which can be used to skip the dump for them.
>
> Personally, I think we can follow this style and add a isinternal column to
> pg_event_trigger and use it to skip the dump.
>

+1. This will not only help pg_dump but also commands like Alter Event
Trigger which enables/disables user-created event triggers but such
ops should be prohibited for internally created event triggers.

-- 
With Regards,
Amit Kapila.

Re: Minimal logical decoding on standbys

[Drouvot, Bertrand]

Hi,

On 4/11/23 10:55 AM, Drouvot, Bertrand wrote:

Hi,

I think we might want to add:

$node_primary->wait_for_replay_catchup($node_standby);

before calling the slot creation.

It's done in the attached, would it be possible to give it a try please?


Actually, let's also wait for the cascading standby to catchup too, like done 
in the attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comdiff --git a/src/test/recovery/t/035_standby_logical_decoding.pl 
b/src/test/recovery/t/035_standby_logical_decoding.pl
index ba98a18bd2..94a8384c31 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -653,9 +653,15 @@ $node_standby->reload;
 $node_primary->psql('postgres', q[CREATE DATABASE testdb]);
 $node_primary->safe_psql('testdb', qq[CREATE TABLE decoding_test(x integer, y 
text);]);
 
+# Wait for the standby to catchup before creating the slots
+$node_primary->wait_for_replay_catchup($node_standby);
+
 # create the logical slots
 create_logical_slots($node_standby, 'promotion_');
 
+# Wait for the cascading standby to catchup before creating the slots
+$node_standby->wait_for_replay_catchup($node_cascading_standby, $node_primary);
+
 # create the logical slots on the cascading standby too
 create_logical_slots($node_cascading_standby, 'promotion_');
 

Re: proposal: psql: show current user in prompt

[Pavel Stehule]

st 5. 4. 2023 v 18:40 odesílatel Pavel Stehule 
napsal:

>
>
> st 5. 4. 2023 v 17:47 odesílatel Robert Haas 
> napsal:
>
>> On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule 
>> wrote:
>> > If the GUC_REPORT should not  be used, then only one possibility is
>> enhancing the protocol, about the possibility to read some predefined
>> server's features from the client.
>> > It can be much cheaper than SQL query, and it can be used when the
>> current transaction is aborted. I can imagine a possibility to read server
>> time or a server session role from a prompt processing routine.
>> >
>> > But for this specific case, you need to cache the role name somewhere.
>> You can simply get oid everytime, but for role name you need to access to
>> system catalogue, and it is not possible in aborted transactions. So at the
>> end, you probably should read "role" GUC.
>> >
>> > Can this design be  acceptable?
>>
>> I don't think we want to add a dedicated protocol message that says
>> "send me the role GUC right now". I mean, we could, but being able to
>> tell the GUC mechanism "please send me the role GUC after every
>> command" sounds a lot easier to use.
>>
>
> I'll try it
>

here is patch with setting GUC_REPORT on role only when it is required by
prompt

Regards

Pavel


>
> Regards
>
> Pavel
>
>
>>
>> --
>> Robert Haas
>> EDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b11d9a6ba3..f774ffa310 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -5401,6 +5401,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
 

 
+   
+ReportGUC (F)
+
+ 
+  
+   Byte1('r')
+   
+
+ Identifies the message type.  ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+   
+  
+
+  
+   String
+   
+
+ The name of GUC option.
+
+   
+  
+
+  
+   Int16
+   
+
+ 1 if reporting should be enables, 0 if reporting should be
+ disabled.
+
+   
+  
+ 
+
+   
+

 RowDescription (B)
 
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index f4f25d1b07..962bf65f95 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4552,7 +4552,24 @@ testdb=> INSERT INTO my_table VALUES (:'content');
 The port number at which the database server is listening.
   
 
-  
+  
+%N
+
+ 
+  The database role name. This value is specified by command
+  SET ROLE. Until execution of this command
+  the value is set to the database session user name.
+ 
+
+ 
+  This substitution requires PostgreSQL
+  version 16 and up. When you use older version, the empty string
+  is used instead.
+ 
+
+  
+
+  
 %n
 
  
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..54a3f5c1e9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -451,6 +451,11 @@ SocketBackend(StringInfo inBuf)
 			doing_extended_query_message = false;
 			break;
 
+		case 'r':/* report GUC */
+			maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
+			doing_extended_query_message = false;
+			break;
+
 		default:
 
 			/*
@@ -4823,6 +4828,24 @@ PostgresMain(const char *dbname, const char *username)
 	pq_flush();
 break;
 
+			case 'r':			/* report GUC */
+{
+	const char	   *name;
+	intcreate_flag;
+
+	name = pq_getmsgstring(&input_message);
+	create_flag = pq_getmsgint(&input_message, 2);
+	pq_getmsgend(&input_message);
+
+	if (create_flag)
+		SetGUCOptionFlag(name, GUC_REPORT);
+	else
+	UnsetGUCOptionFlag(name, GUC_REPORT);
+
+	send_ready_for_query = true;
+}
+break;
+
 			case 'S':			/* sync */
 pq_getmsgend(&input_message);
 finish_xact_command();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea67cfa5e5..ee8d831e1c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2532,6 +2532,37 @@ BeginReportingGUCOptions(void)
 	}
 }
 
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+	struct config_generic *conf;
+
+	/* only GUC_REPORT flag is supported now */
+	Assert(flag == GUC_REPORT);
+
+	conf = find_option(name, false, true, ERROR);
+	conf->flags |= flag;
+
+	if (flag == GUC_REPORT)
+		/* force transmit value of related option to client Parameter Status */
+		ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+	struct config_generic *conf;
+
+	/* only GUC_REPORT flag is supported now */
+	Assert(flag == GUC_REPORT);
+
+	conf = find_option(n

Re: Non-superuser subscription owners

[Amit Kapila]

On Mon, Apr 10, 2023 at 9:15 PM Robert Haas  wrote:
>
> On Sat, Apr 8, 2023 at 1:35 AM Amit Kapila  wrote:
> > Do we need to have a check for this new option "password_required" in
> > maybe_reread_subscription() where we "Exit if any parameter that
> > affects the remote connection was changed."? This new option is
> > related to the remote connection so I thought it is worth considering
> > whether we want to exit and restart the apply worker when this option
> > is changed.
>
> Hmm, good question. I think that's probably a good idea. If the
> current connection is already working, the only possible result of
> getting rid of it and trying to create a new one is that it might now
> fail instead, but someone might want that behavior. Otherwise, they'd
> instead find the failure at a later, maybe less convenient, time.
>

I think additionally, we should check that the new owner of the
subscription is not a superuser, otherwise, anyway, this parameter is
ignored. Please find the attached to add this check.

-- 
With Regards,
Amit Kapila.


exit_on_sub_opt_change_1.patch
Description: Binary data

Re: User functions for building SCRAM secrets

[Magnus Hagander]

On Fri, Mar 24, 2023 at 4:48 PM Daniel Gustafsson  wrote:
>
> > On 22 Mar 2023, at 15:06, Jonathan S. Katz  wrote:
> > On 3/22/23 2:48 AM, Michael Paquier wrote:
>
> >> I was reading this thread again, and pondered on this particular
> >> point:
> >> https://www.postgresql.org/message-id/caawbhmhjcfc4oaga_7yluhtj6j+rxey+bodrygzndaflgfz...@mail.gmail.com
> >> We've had our share of complains over the years that Postgres logs
> >> password data in the logs with various DDLs, so I'd tend to agree that
> >> this is not a practice we should try to encourage more.  The
> >> parameterization of the SCRAM verifiers through GUCs (like Daniel's
> >> https://commitfest.postgresql.org/42/4201/ for the iteration number)
> >> is more promising because it is possible to not have to send the
> >> password over the wire with once we let libpq take care of the
> >> computation, and the server would not know about that
> >
> > I generally agree with not allowing password data to be in logs, but in 
> > practice, there are a variety of tools and extensions that obfuscate or 
> > remove passwords from PostgreSQL logs. Additionally, this function is not 
> > targeted for SQL statements directly, but stored procedures.
> >
> > For example, an extension like "pg_tle" exposes the ability for someone to 
> > write a "check_password_hook" directly from PL/pgSQL[1] (and other 
> > languages). As we've made it a best practice to pre-hash the password on 
> > the client-side, a user who wants to write a check password hook against a 
> > SCRAM verifier needs to be able to compare the verifier against some 
> > existing set of plaintext criteria, and has to write their own function to 
> > do it. I have heard several users who have asked to do this, and the only 
> > feedback I can give them is "implement your own SCRAM build secret 
> > function."
>
> I'm not sure I follow; doesn't this - coupled with this patch - imply passing
> the plaintext password from client to the server, hashing it with a known salt
> and comparing this with something in plaintext hashed with the same known 
> salt?
> If so, that's admittedly not a usecase I am terribly excited about.  My
> preference is to bring password checks to the plaintext password, not bring 
> the
> plaintext password to the password check.

Given how much we marketed, for good reasons, SCRAM as a way to
finally make postgres *not* do this, it seems like a really strange
path to take to go back to doing it again.

Having the function always generate a random salt seems more
reasonable though, and would perhaps be something that helps in some
of the cases? It won't help with the password policy one, as it's too
secure for that, but it would help with the postgres-is-the-client
one?


> > And, if my PostgreSQL server _is_ the client, e.g. it's making a dblink 
> > call to another PostgreSQL server, the only way it can modify a password is 
> > by sending the plaintext credential over the wire.
>
> My experience with dblink setups is too small to have much insight here, but I
> (perhaps naively) assumed that dblink setups generally involved two servers
> under the same control making such changes be possible out of band.

I have seen a few, and certainly more FDW based ones these days. But
I'm not sure I've come across one yet where one wants to *change the
password* through dblink. Since it's server-to-server, most people
would just change the password on the target server and then update
the FDW/dblink configuration with the new password. (Of course, the
storage of that password on the FDW/dblink layer is a terrible thing
in the first place from a security perspective, but it's what we
have).


> > Maybe I'm not conveying the problem this is solving -- I'm happy to go one 
> > more round trying to make it clearer -- but if this is not clear, it'd be 
> > good to at develop an alternative approach to this before withdrawing the 
> > patch.
>
> If this is mainly targeting extension use, is there a way in which an 
> extension
> could have all this functionality with no: a) not exposing any of it from the
> server; b) not having to copy/paste lots into the extension and; c) have a
> reasonable way to keep up with any changes made in the backend?

I realize I forgot to write this reply back when the thread was alive,
so here's a zombie-awakener!

One way to accomplish this would be to create a new predefined role,
say pg_change_own_password, by default granted to public. When a user
is a member of this role they can, well, change their own password.
And it will be done using the full security of scram, without cutting
anything. For those that want to enforce a password policy or anything
else that requires the server to see the cleartext or
cleartext-equivalent of the password can then revoke this role from
public, and force password changes to go through a security definer
funciton, like SELECT pg_change_password_with_policy('joe',
'mysupersecretpasswrod').

This function can then be under 

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thank you for giving comments! PSA new version.

> ==
> doc/src/sgml/ref/pgupgrade.sgml
> 
> 1.
> + 
> +  --include-logical-replication-slots
> +  
> +   
> +Upgrade logical replication slots. Only permanent replication slots
> +included. Note that pg_upgrade does not check the installation of
> +plugins.
> +   
> +  
> + 
> 
> Missing word.
> 
> "Only permanent replication slots included." --> "Only permanent
> replication slots are included."

Fixed.

> ==
> src/bin/pg_dump/pg_dump.c
> 
> 2. help
> 
> @@ -1119,6 +1145,8 @@ help(const char *progname)
>   printf(_("  --no-unlogged-table-data do not dump unlogged table
> data\n"));
>   printf(_("  --on-conflict-do-nothing add ON CONFLICT DO NOTHING
> to INSERT commands\n"));
>   printf(_("  --quote-all-identifiers  quote all identifiers, even
> if not key words\n"));
> + printf(_("  --logical-replication-slots-only\n"
> + "   dump only logical replication slots,
> no schema or data\n"));
>   printf(_("  --rows-per-insert=NROWS  number of rows per INSERT;
> implies --inserts\n"));
> A previous review comment ([1] #11b) seems to have been missed. This
> help is misplaced. It should be in alphabetical order consistent with
> all the other help.

Sorry, fixed.

> src/bin/pg_dump/pg_dump.h
> 
> 3. _LogicalReplicationSlotInfo
> 
> +/*
> + * The LogicalReplicationSlotInfo struct is used to represent replication
> + * slots.
> + * XXX: add more attrbutes if needed
> + */
> +typedef struct _LogicalReplicationSlotInfo
> +{
> + DumpableObject dobj;
> + char*plugin;
> + char*slottype;
> + char*twophase;
> +} LogicalReplicationSlotInfo;
> +
> 
> 4a.
> The indent of the 'LogicalReplicationSlotInfo' looks a bit strange,
> unlike others in this file. Is it OK?

I was betrayed by pgindent because of the reason you pointed out.
Fixed.

> 4b.
> There was no typedefs.list file in this patch. Maybe the above
> whitespace problem is a result of that omission.

Your analysis is correct. Added.

> .../pg_upgrade/t/003_logical_replication.pl
> 
> 5.
> 
> +# Run pg_upgrade. pg_upgrade_output.d is removed at the end
> +command_ok(
> + [
> + 'pg_upgrade', '--no-sync',
> + '-d', $old_publisher->data_dir,
> + '-D', $new_publisher->data_dir,
> + '-b', $bindir,
> + '-B', $bindir,
> + '-s', $new_publisher->host,
> + '-p', $old_publisher->port,
> + '-P', $new_publisher->port,
> + $mode,'--include-logical-replication-slot'
> + ],
> + 'run of pg_upgrade for new publisher');
> 
> 5a.
> How can this test even be working as-expected with those options?
> 
> Here it is passing option '--include-logical-replication-slot' but
> AFAIK the proper option name everywhere else in this patch is
> '--include-logical-replication-slots' (with the 's')

This is because getopt_long implemented by GNU can accept incomplete options if
collect one can be predicted from input. E.g. pg_upgrade on linux can accept
`--ve` as `--verbose`, whereas the binary built on Windows cannot.

Anyway, the difference was not my expectation. Fixed.

> 5b.
> I'm not sure that "pg_upgrade for new publisher" makes sense.
> 
> It's more like "pg_upgrade of old publisher", or simply "pg_upgrade of
> publisher"
>

Fixed.

Additionally, I fixed two bugs which are detected by AddressSanitizer.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v4-0001-pg_upgrade-Add-include-logical-replication-slots-.patch
Description:  v4-0001-pg_upgrade-Add-include-logical-replication-slots-.patch

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thank you for giving explanation.

> 
> Hopefully, someone will correct me if this explanation is wrong, but
> my understanding of the different prefixes is like this --
> 
> "XXX" is used as a marker for future developers to consider maybe
> revisiting/improving something that the comment refers to
> e.g.
> /* XXX - it would be better to code this using blah but for now we did
> not */
> /* XXX - option 'foo' is not currently supported but... */
> /* XXX - it might be worth considering adding more checks or an assert
> here because... */
> 
> OTOH, "Note" is just for highlighting why something is the way it is,
> but with no implication that it should be revisited/changed in the
> future.
> e.g.
> /* Note: We deliberately do not test the state here because... */
> /* Note: This memory must be zeroed because... */
> /* Note: This string has no '\0' terminator so... */

I confirmed that current "XXX" comments must be removed by improving
or some decision. Therefore I kept current annotation.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Missing wait_for_replay_catchup in 035_standby_logical_decoding.pl

[Drouvot, Bertrand]

hi hackers,

while working on the issue reported by Noah in [1], I realized that there is an
issue in 035_standby_logical_decoding.pl.

The issue is here:

"
$node_standby->reload;

$node_primary->psql('postgres', q[CREATE DATABASE testdb]);
$node_primary->safe_psql('testdb', qq[CREATE TABLE decoding_test(x integer, y 
text);]);

# create the logical slots
create_logical_slots($node_standby, 'promotion_');

# create the logical slots on the cascading standby too
create_logical_slots($node_cascading_standby, 'promotion_');
"

We are not waiting for the standby/cascading standby to catchup (so that the 
create
database get replicated) before creating the replication slots (in testdb).

While, It's still not 100% sure that it will fix Noah's issue, I think this has 
to be fixed.

Please find, attached a patch proposal to do so.

Regards,

[1]: 
https://www.postgresql.org/message-id/20230411053657.GA1177147%40rfd.leadboat.com
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comdiff --git a/src/test/recovery/t/035_standby_logical_decoding.pl 
b/src/test/recovery/t/035_standby_logical_decoding.pl
index ba98a18bd2..94a8384c31 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -653,9 +653,15 @@ $node_standby->reload;
 $node_primary->psql('postgres', q[CREATE DATABASE testdb]);
 $node_primary->safe_psql('testdb', qq[CREATE TABLE decoding_test(x integer, y 
text);]);
 
+# Wait for the standby to catchup before creating the slots
+$node_primary->wait_for_replay_catchup($node_standby);
+
 # create the logical slots
 create_logical_slots($node_standby, 'promotion_');
 
+# Wait for the cascading standby to catchup before creating the slots
+$node_standby->wait_for_replay_catchup($node_cascading_standby, $node_primary);
+
 # create the logical slots on the cascading standby too
 create_logical_slots($node_cascading_standby, 'promotion_');
 

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear hackers,

My PoC does not read and copy logical mappings files to new node, but I
did not analyzed in detail whether it is correct. Now I have done this and
considered that they do not have to be copied because transactions which 
executed
at the same time as rewriting are no longer decoded. How do you think?
Followings my analysis.

## What is logical mappings files?

Logical mappings file is used to track the system catalogs while logical 
decoding
even if its heap file is written. Sometimes catalog heaps files are modified, or
completely replaced to new files via VACUUM FULL or CLUSTER, but reorder buffer
cannot not track new one as-is. Mappings files allow to do them.

The file contains key-value relations for old-to-new tuples. Also, the name of
files contains the LSN where the triggered event is happen.

Mappings files are needed when transactions which modify catalogs are decoded.
If the LSN of files are older than restart_lsn, they are no longer needed then
removed. Please see CheckPointLogicalRewriteHeap().

## Is it needed?

I think this is not needed.
Currently pg_upgrade dumps important information from old publisher and then
execute pg_create_logical_replication_slot() on new one. Apart from
pg_copy_logical_replication_slot(), retart_lsn and confirmed_flush_lsn for old
slot is not taken over to the new slot. They are recalculated on new node while
creating. This means that transactions which have modified catalog heaps on the
old publisher are no longer decoded on new publisher.

Therefore, the mappings files on old publisher are not needed for new one.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: When to drop src/tools/msvc support

[Andrew Dunstan]

On 2023-04-11 Tu 04:05, Dave Page wrote:



On Tue, 11 Apr 2023 at 08:09, Magnus Hagander  wrote:

On Tue, Apr 11, 2023 at 12:27 AM Andres Freund
 wrote:
>
> Hi,
>
> On 2023-04-10 19:55:35 +0100, Dave Page wrote:
> > Projects other than the EDB installers use the MSVC build
system - e.g.
> > pgAdmin uses it’s own builds of libpq and other tools (psql,
pg_dump etc)
> > that are pretty heavily baked into a fully automated build
system (even the
> > build servers and all their requirements are baked into Ansible).
> >
> > Changing that lot would be non-trivial, though certainly
possible, and I
> > suspect we’re not the only ones doing that sort of thing.
>
> Do you have a link to the code for that, if it's open? Just to
get an
> impression for how hard it'd be to switch over?


The pgadmin docs/readme refers to
https://github.com/pgadmin-org/pgadmin4/tree/master/pkg/win32

It clearly doesn't have the full automation stuff, but appears to have
the parts about building the postgres dependency.


Yeah, that's essentially the manual process, though I haven't tested 
it in a while. The Ansible stuff is not currently public. I suspect 
(or rather, hope) that we can pull in all the additional packages 
required using Chocolatey which shouldn't be too onerous.


Probably my main concern is that the Meson build can use the same 
version of the VC++ compiler that we use (v14), which is carefully 
matched for compatibility with all the various components, just in 
case anything passes CRT pointers around. Python is the one thing we 
don't build ourselves on Windows and the process will build modules 
like gssapi and psycopg (which links with libpq of course), so we're 
basically following what they use.





For meson you just need to to "pip install meson ninja" in your python 
distro and you should be good to go (they will be installed in python's 
Scripts directory). Don't use chocolatey to install meson/ninja - I ran 
into issues doing that.


AFAICT meson will use whatever version of VC you have installed, 
although I have only been testing with VC2019.



cheers


andrew

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

Re: fairywren exiting in ecpg

[Andrew Dunstan]

On 2023-04-04 Tu 08:22, Andrew Dunstan wrote:



On 2023-04-03 Mo 21:15, Andres Freund wrote:

Hi,

Looks like fairywren is possibly seeing something I saw before and spent many
days looking into:
https://postgr.es/m/20220909235836.lz3igxtkcjb5w7zb%40awork3.anarazel.de
which led me to add the following to .cirrus.yml:

 # Cirrus defaults to SetErrorMode(SEM_NOGPFAULTERRORBOX | ...). That
 # prevents crash reporting from working unless binaries do SetErrorMode()
 # themselves. Furthermore, it appears that either python or, more likely,
 # the C runtime has a bug where SEM_NOGPFAULTERRORBOX can very
 # occasionally *trigger* a crash on process exit - which is hard to debug,
 # given that it explicitly prevents crash dumps from working...
 # 0x8001 is SEM_FAILCRITICALERRORS | SEM_NOOPENFILEERRORBOX
 CIRRUS_WINDOWS_ERROR_MODE: 0x8001


The mingw folks also spent a lot of time looking into this ([1]), without a
lot of success.

It sure looks like it might be a windows C runtime issue - none of the
stacktrace handling python has gets invoked. I could not find any relevant
behavoural differences in python's code that depend on SEM_NOGPFAULTERRORBOX
being set.

It'd be interesting to see if fairywren's occasional failures go away if you
set MSYS=winjitdebug (which prevents msys from adding SEM_NOGPFAULTERRORBOX to
ErrorMode).



trying now. Since this happened every build or so it shouldn't take 
long for us to see.


(I didn't see anything in the MSYS2 docs that specified the possible 
values for MSYS :-( )






The error hasn't been seen since I set this about a week ago.


cheers


andrew


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

Re: When to drop src/tools/msvc support

[Dave Page]

On Tue, 11 Apr 2023 at 11:58, Andrew Dunstan  wrote:

>
> On 2023-04-11 Tu 04:05, Dave Page wrote:
>
>
>
> On Tue, 11 Apr 2023 at 08:09, Magnus Hagander  wrote:
>
>> On Tue, Apr 11, 2023 at 12:27 AM Andres Freund 
>> wrote:
>> >
>> > Hi,
>> >
>> > On 2023-04-10 19:55:35 +0100, Dave Page wrote:
>> > > Projects other than the EDB installers use the MSVC build system -
>> e.g.
>> > > pgAdmin uses it’s own builds of libpq and other tools (psql, pg_dump
>> etc)
>> > > that are pretty heavily baked into a fully automated build system
>> (even the
>> > > build servers and all their requirements are baked into Ansible).
>> > >
>> > > Changing that lot would be non-trivial, though certainly possible,
>> and I
>> > > suspect we’re not the only ones doing that sort of thing.
>> >
>> > Do you have a link to the code for that, if it's open? Just to get an
>> > impression for how hard it'd be to switch over?
>>
>>
>> The pgadmin docs/readme refers to
>> https://github.com/pgadmin-org/pgadmin4/tree/master/pkg/win32
>>
>> It clearly doesn't have the full automation stuff, but appears to have
>> the parts about building the postgres dependency.
>>
>
> Yeah, that's essentially the manual process, though I haven't tested it in
> a while. The Ansible stuff is not currently public. I suspect (or rather,
> hope) that we can pull in all the additional packages required using
> Chocolatey which shouldn't be too onerous.
>
> Probably my main concern is that the Meson build can use the same version
> of the VC++ compiler that we use (v14), which is carefully matched for
> compatibility with all the various components, just in case anything passes
> CRT pointers around. Python is the one thing we don't build ourselves on
> Windows and the process will build modules like gssapi and psycopg (which
> links with libpq of course), so we're basically following what they use.
>
>
>
> For meson you just need to to "pip install meson ninja" in your python
> distro and you should be good to go (they will be installed in python's
> Scripts directory). Don't use chocolatey to install meson/ninja - I ran
> into issues doing that.
>
> AFAICT meson will use whatever version of VC you have installed, although
> I have only been testing with VC2019.
>
OK, that sounds easy enough then (famous last words!)

Thanks!

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: When to drop src/tools/msvc support

[Jonathan S. Katz]

On 4/11/23 7:54 AM, Dave Page wrote:



On Tue, 11 Apr 2023 at 11:58, Andrew Dunstan > wrote:


For meson you just need to to "pip install meson ninja" in your
python distro and you should be good to go (they will be installed
in python's Scripts directory). Don't use chocolatey to install
meson/ninja - I ran into issues doing that.

AFAICT meson will use whatever version of VC you have installed,
although I have only been testing with VC2019.

OK, that sounds easy enough then (famous last words!)


[RMT hat]

Dave -- does this mean you see a way forward on moving the Windows 
builds over to use Meson instead of MSVC?


Do you think we'll have enough info by end of this week to make a 
decision on whether we can drop MSVC in v16?


Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: Unnecessary confirm work on logical replication

[Emre Hasegeli]

> In fact, the WAL sender always starts reading WAL from restart_lsn,
> which in turn is always <= confirmed_flush_lsn. While reading WAL, WAL
> sender may read XLOG_RUNNING_XACTS WAL record with lsn <=
> confirmed_flush_lsn. While processing XLOG_RUNNING_XACTS record it may
> update its restart_lsn and catalog_xmin with current_lsn = lsn fo
> XLOG_RUNNING_XACTS record. In this situation current_lsn <=
> confirmed_flush_lsn.

This can only happen when the WAL sender is restarted.  However in
this case, the restart_lsn and catalog_xmin should have already been
persisted by the previous run of the WAL sender.

I still doubt these calls are necessary.  I think there is a
complicated chicken and egg problem here.  Here is my logic:

1) LogicalConfirmReceivedLocation() is called explicitly when
confirmed_flush is sent by the replication client.

2) LogicalConfirmReceivedLocation() is the only place that updates
confirmed_flush.

3) The replication client can only send a confirmed_flush for a
current_lsn it has already received.

4) These two functions have already run for any current_lsn the
replication client has received.

5) These two functions call LogicalConfirmReceivedLocation() only if
current_lsn <= confirmed_flush.

Thank you for your patience.

Re: An oversight in ExecInitAgg for grouping sets

[David Rowley]

On Mon, 9 Jan 2023 at 22:21, David Rowley  wrote:
> One extra thing I noticed was that I had to add a new
> VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
> the freelist. I didn't quite manage to figure out why that's needed as
> when we do AllocSetFree() we don't mark the pfree'd memory with
> NOACCESS, and it also looks like AllocSetReset() sets the keeper
> block's memory to NOACCESS, but that function also clears the
> freelists too, so the freelist chunk is not coming from a recently
> reset context.

It seems I didn't look hard enough for NOACCESS marking. It's in
wipe_mem(). So that explains why the VALGRIND_MAKE_MEM_DEFINED is
required in AllocSetAlloc.

Since this patch really only touches Valgrind macros, I don't really
feel like there's a good reason we can't still do this for v16, but
I'll start another thread to increase visibility to see if anyone else
thinks differently about that.

David

Re: When to drop src/tools/msvc support

[Dave Page]

On Tue, 11 Apr 2023 at 13:52, Jonathan S. Katz  wrote:

> On 4/11/23 7:54 AM, Dave Page wrote:
> >
> >
> > On Tue, 11 Apr 2023 at 11:58, Andrew Dunstan  > > wrote:
> >
> > For meson you just need to to "pip install meson ninja" in your
> > python distro and you should be good to go (they will be installed
> > in python's Scripts directory). Don't use chocolatey to install
> > meson/ninja - I ran into issues doing that.
> >
> > AFAICT meson will use whatever version of VC you have installed,
> > although I have only been testing with VC2019.
> >
> > OK, that sounds easy enough then (famous last words!)
>
> [RMT hat]
>
> Dave -- does this mean you see a way forward on moving the Windows
> builds over to use Meson instead of MSVC?
>

I can see a way forward, yes.


>
> Do you think we'll have enough info by end of this week to make a
> decision on whether we can drop MSVC in v16?
>

There's no way I can test anything this week - I'm on leave for most of it
and AFK.

But, my point was more that there are almost certainly more projects using
the MSVC build system than the EDB installers; pgAdmin being just one
example.

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Protecting allocator headers with Valgrind

[David Rowley]

Over on [1], Tom mentioned that we might want to rethink the decision
to not protect chunk headers with Valgrind.  That thread fixed a bug
that was accessing array element -1, which effectively was reading the
MemoryChunk at the start of the allocated chunk as an array element.

I wrote a patch to adjust the Valgrind macros to mark the MemoryChunks
as NOACCESS and that finds the bug reported on that thread (with the
fix for it reverted).

I didn't quite get a clear run at committing the changes during the
v16 cycle, but wondering since they're really just Valgrind macro
changes if anyone would object to doing it now?

I know there are a few people out there running sqlsmith and/or
sqlancer under Valgrind. It would be good to have this in so we could
address any new issues the attached patch might help them highlight.

Any objections?

(Copying in Tom and Richard same as original thread.  Reposting for
more visibility of this change)

David


protect_MemoryChunks_with_Valgrind.patch
Description: Binary data

Re: is_superuser is not documented

[Fujii Masao]

On 2023/04/08 23:53, Joseph Koshakow wrote:



On Mon, Apr 3, 2023 at 10:47 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com>> wrote:
 >    Yes, the patch has not been committed yet because of lack of review 
comments.
 >    Do you have any review comments on this patch?
 >    Or you think it's ready for committer?

I'm not very familiar with this code, so I'm not sure how much my
review is worth, but maybe it will spark some discussion.


Thanks for the comments!



 > Yes, this patch moves the descriptions of is_superuser to config.sgml
 > and changes its group to PRESET_OPTIONS.

is_superuser feels a little out of place in this file. All of
the options here apply to the entire PostgreSQL server, while
is_superuser only applies to the current session.


Aren't other preset options like lc_collate, lc_ctype and server_encoding
similar to is_superuser? They seem to behave in a similar way as their
settings can be different for each connection depending on the connected 
database.



I'm not familiar with the origins of is_superuser and it may be too
late for this, but it seems like is_superuser would fit in much better
as a system information function [0] rather than a GUC. Particularly
in the Session Information Functions.


I understand your point, but I think it would be more confusing to document
is_superuser there because it's defined and behaves differently from
session information functions like current_user. For instance,
the value of is_superuser can be displayed using SHOW command,
while current_user cannot. Therefore, it's better to keep is_superuser
separate from the session information functions.



As a side note server_version, server_encoding, lc_collate, and
lc_ctype all appear in both the preset options section of config.sgml
and in show.sgml. I'm not sure what the logic is for just including
these three parameters in show.sgml, but I think we should either
include all of the preset options or none of them.


Agreed. I think that it's better to just treat them as GUCs and
remove their descriptions from show.sgml.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: When to drop src/tools/msvc support

[Tom Lane]

Dave Page  writes:
> On Tue, 11 Apr 2023 at 13:52, Jonathan S. Katz  wrote:
>> Do you think we'll have enough info by end of this week to make a
>> decision on whether we can drop MSVC in v16?

> There's no way I can test anything this week - I'm on leave for most of it
> and AFK.
> But, my point was more that there are almost certainly more projects using
> the MSVC build system than the EDB installers; pgAdmin being just one
> example.

Yeah.  Even if EDB can manage this, we're talking about going from
"src/tools/msvc is the only option" in v15 to "meson is the only
option" in v16.  That seems pretty abrupt.  Notably, it's assuming
that there are no big problems in the meson build system that will
take awhile to fix once discovered by users.  That's a large
assumption for code that hasn't even reached beta yet.

Sadly, I think we really have to ship both build systems in v16.

regards, tom lane

Re: ERROR messages in VACUUM's PARALLEL option

[Tom Lane]

David Rowley  writes:
> Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> BUFFER_USAGE_LIMIT option.

> 1) buffer_usage_limit in the ERROR messages should be consistently in 
> uppercase.

FWIW, I think this is exactly backward, and so is whatever code you
based this on.  Our usual habit is to write GUC names and suchlike
in lower case in error messages.  Add double quotes if you feel you
want to set them off from the surrounding text.  Here's a typical
example of longstanding style:

regression=# set max_parallel_workers_per_gather to -1;
ERROR:  -1 is outside the valid range for parameter 
"max_parallel_workers_per_gather" (0 .. 1024)

regards, tom lane

Re: When to drop src/tools/msvc support

[Jonathan S. Katz]

On 4/11/23 9:49 AM, Tom Lane wrote:

Dave Page  writes:

On Tue, 11 Apr 2023 at 13:52, Jonathan S. Katz  wrote:

Do you think we'll have enough info by end of this week to make a
decision on whether we can drop MSVC in v16?



There's no way I can test anything this week - I'm on leave for most of it
and AFK.
But, my point was more that there are almost certainly more projects using
the MSVC build system than the EDB installers; pgAdmin being just one
example.


Yeah.  Even if EDB can manage this, we're talking about going from
"src/tools/msvc is the only option" in v15 to "meson is the only
option" in v16.  That seems pretty abrupt.  Notably, it's assuming
that there are no big problems in the meson build system that will
take awhile to fix once discovered by users.  That's a large
assumption for code that hasn't even reached beta yet.

[Personal hat]

We'll probably see some of this for non-Windows builds, too. Granted, 
autoconf still seems to work, at least based on my tests.



Sadly, I think we really have to ship both build systems in v16.


[Personal hat]

I've come to this conclusion, too -- it does mean 5 more years of 
supporting it.


But maybe we can make it clear in the release notes + docs that this is 
slated for deprecation and will be removed from v17? That way we can say 
"we provided ample warning to move to the new build system."


Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: When to drop src/tools/msvc support

[Tom Lane]

"Jonathan S. Katz"  writes:
> On 4/11/23 9:49 AM, Tom Lane wrote:
>> Sadly, I think we really have to ship both build systems in v16.

> But maybe we can make it clear in the release notes + docs that this is 
> slated for deprecation and will be removed from v17? That way we can say 
> "we provided ample warning to move to the new build system."

Yes, we absolutely should do that, as already discussed upthread.

regards, tom lane

v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized

[Justin Pryzby]

Reduced from sqlsmith, this query fails under debug_parallel_query=1.

The elog was added at: 55416b26a98fcf354af88cdd27fc2e045453b68a
But (I'm not sure) the faulty commit may be 8edd0e7946 (Suppress Append
and MergeAppend plan nodes that have a single child).

postgres=# SET force_parallel_mode =1; CREATE TABLE x (i int) PARTITION BY 
RANGE (i); CREATE TABLE x1 PARTITION OF x DEFAULT ;
 select * from pg_class,
  lateral (select pg_catalog.bit_and(1)
  from pg_class as sample_1
  where case when EXISTS (
select 1 from x
  where EXISTS (
select 1 from pg_catalog.pg_depend
  where (sample_1.reltuples is NULL)
  )) then 1 end
   is NULL)x
where false;

-- 
Justin

Various typo fixes

[Thom Brown]

Hi,

I've attached a patch with a few typo and grammatical fixes.

Regards

Thom


various_typos_and_grammar_fixes.patch
Description: Binary data

Re: Various typo fixes

[Justin Pryzby]

On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote:
> I've attached a patch with a few typo and grammatical fixes.

I think you actually sent the "git-diff" manpage :(

-- 
Justin

Re: Show various offset arrays for heap WAL records

[Melanie Plageman]

Hi,

static void
infobits_desc(StringInfo buf, uint8 infobits, const char *keyname)
{
appendStringInfo(buf, "%s: [", keyname);

Why can we assume that there will be no space at the end here?

I know we need to be able to avoid doing the comma overwriting if no
flags were set. In general, we expect record description elements to
prepend themselves with commas and spaces, but these infobits, for
example, use a trailing comma and space. If someone adds a description
element with a trailing space, they will trip this assert. We should at
least add a comment explaining this assertion so someone knows what to
do if they trip it.

Otherwise, we can return early if no flags are set. That will probably
make for slightly messier code since we would still have to construct
the empty list.

Assert(buf->data[buf->len - 1] != ' ');

if (infobits & XLHL_XMAX_IS_MULTI)
appendStringInfoString(buf, "IS_MULTI, ");
if (infobits & XLHL_XMAX_LOCK_ONLY)
appendStringInfoString(buf, "LOCK_ONLY, ");
if (infobits & XLHL_XMAX_EXCL_LOCK)
appendStringInfoString(buf, "EXCL_LOCK, ");
if (infobits & XLHL_XMAX_KEYSHR_LOCK)
appendStringInfoString(buf, "KEYSHR_LOCK, ");
if (infobits & XLHL_KEYS_UPDATED)
appendStringInfoString(buf, "KEYS_UPDATED, ");

if (buf->data[buf->len - 1] == ' ')
{
/* Truncate-away final unneeded ", "  */
Assert(buf->data[buf->len - 2] == ',');
buf->len -= 2;
buf->data[buf->len] = '\0';
}

appendStringInfoString(buf, "]");
}

Also you didn't add the same assert to truncate_flags_desc().

static void
truncate_flags_desc(StringInfo buf, uint8 flags)
{
appendStringInfoString(buf, "flags: [");

if (flags & XLH_TRUNCATE_CASCADE)
appendStringInfoString(buf, "CASCADE, ");
if (flags & XLH_TRUNCATE_RESTART_SEQS)
appendStringInfoString(buf, "RESTART_SEQS, ");

if (buf->data[buf->len - 1] == ' ')
{
/* Truncate-away final unneeded ", "  */
Assert(buf->data[buf->len - 2] == ',');
buf->len -= 2;
buf->data[buf->len] = '\0';
}

appendStringInfoString(buf, "]");
}

Not the fault of this patch, but I also noticed that heap UPDATE and
HOT_UPDATE records have xmax twice and don't differentiate between new
and old. I think that was probably a mistake.

description  | off: 119, xmax: 1105, flags: 0x00, old_infobits:
[], new off: 100, xmax 0

else if (info == XLOG_HEAP_UPDATE)
{
xl_heap_update *xlrec = (xl_heap_update *) rec;

appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
xlrec->old_offnum,
xlrec->old_xmax,
xlrec->flags);
infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
appendStringInfo(buf, ", new off: %u, xmax %u",
xlrec->new_offnum,
xlrec->new_xmax);
}
else if (info == XLOG_HEAP_HOT_UPDATE)
{
xl_heap_update *xlrec = (xl_heap_update *) rec;

appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
xlrec->old_offnum,
xlrec->old_xmax,
xlrec->flags);
infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
appendStringInfo(buf, ", new off: %u, xmax: %u",
xlrec->new_offnum,
xlrec->new_xmax);
}

Also not the fault of this patch, but looking at the output while using
this, I realized truncate record type has a stringified version of its
flags while other record types, like update, don't. Do you think this
makes sense? Perhaps not something we can change now, though...

description  | off: 1, xmax: 1183, flags: 0x00, old_infobits: [],
new off: 119, xmax 0

Also not the fault of this patch, but I noticed that leaftopparent is
often InvalidBlockNumber--which shows up as 4294967295. I wonder if
anyone would be confused by this. Maybe devs know that this value is
InvalidBlockNumber. In the future, perhaps we should interpolate the
string "InvalidBlockNumber"?

description | left: 436, right: 389, level: 0, safexid: 0:1091,
leafleft: 436, leafright: 389, leaftopparent: 4294967295

- Melanie

Re: Various typo fixes

[Thom Brown]

On Tue, 11 Apr 2023 at 15:39, Justin Pryzby  wrote:
>
> On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote:
> > I've attached a patch with a few typo and grammatical fixes.
>
> I think you actually sent the "git-diff" manpage :(

Oh dear, well that's a first.  Thanks for pointing out.

Re-attached.

Thom


various_typos_and_grammar_fixes.patch
Description: Binary data

Re: When to drop src/tools/msvc support

[Jonathan S. Katz]

On 4/11/23 10:12 AM, Tom Lane wrote:

"Jonathan S. Katz"  writes:

On 4/11/23 9:49 AM, Tom Lane wrote:

Sadly, I think we really have to ship both build systems in v16.



But maybe we can make it clear in the release notes + docs that this is
slated for deprecation and will be removed from v17? That way we can say
"we provided ample warning to move to the new build system."


Yes, we absolutely should do that, as already discussed upthread.


Ah yes, I saw Andres' notep[1] yesterday and had already forgotten. +1 
on that plan.


Jonathan

[1] 
https://www.postgresql.org/message-id/20230410223219.4tllxhz3hgwhh4tm%40awork3.anarazel.de


OpenPGP_signature
Description: OpenPGP digital signature

Re: Non-superuser subscription owners

[Robert Haas]

On Tue, Apr 11, 2023 at 5:53 AM Amit Kapila  wrote:
> I think additionally, we should check that the new owner of the
> subscription is not a superuser, otherwise, anyway, this parameter is
> ignored. Please find the attached to add this check.

I don't see why we should check that. It makes this different from all
the other cases and I don't see any benefit.

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

Re: Various typo fixes

[Justin Pryzby]

On Tue, Apr 11, 2023 at 03:43:12PM +0100, Thom Brown wrote:
> On Tue, 11 Apr 2023 at 15:39, Justin Pryzby  wrote:
> >
> > On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote:
> > > I've attached a patch with a few typo and grammatical fixes.
> >
> > I think you actually sent the "git-diff" manpage :(
> 
> Oh dear, well that's a first.  Thanks for pointing out.

Thanks.  I think these are all new in v16, right ?

I noticed some of these too - I'll send a patch pretty soon.

|+++ b/doc/src/sgml/logicaldecoding.sgml
|@@ -326,11 +326,11 @@ postgres=# select * from 
pg_logical_slot_get_changes('regression_slot', NULL, NU
|  connection is alive (for example a node restart would break it). Then, 
the
|  primary may delete system catalog rows that could be needed by the 
logical
|  decoding on the standby (as it does not know about the catalog_xmin on 
the
|- standby). Existing logical slots on standby also get invalidated if 
wal_level
|- on primary is reduced to less than 'logical'. This is done as soon as the
|- standby detects such a change in the WAL stream. It means, that for 
walsenders
|- that are lagging (if any), some WAL records up to the wal_level 
parameter change
|- on the primary won't be decoded.
|+ standby). Existing logical slots on standby also get invalidated if
|+ wal_level on the primary is reduced to less than 
'logical'.
|+ This is done as soon as the standby detects such a change in the WAL 
stream.
|+ It means that, for walsenders which are lagging (if any), some WAL 
records up
|+ to the wal_level parameter change on the primary won't be decoded.
| 

I think "logical" should be a  here.

Re: Should vacuum process config file reload more often

[Masahiko Sawada]

On Fri, Apr 7, 2023 at 10:23 PM Daniel Gustafsson  wrote:
>
> > On 7 Apr 2023, at 15:07, Melanie Plageman  wrote:
> > On Fri, Apr 7, 2023 at 2:53 AM Masahiko Sawada  
> > wrote:
>
> >> +   /* Only log updates to cost-related variables */
> >> +   if (vacuum_cost_delay == original_cost_delay &&
> >> +   vacuum_cost_limit == original_cost_limit)
> >> +   return;
> >>
> >> IIUC by default, we log not only before starting the vacuum but also
> >> when changing cost-related variables. Which is good, I think, because
> >> logging the initial values would also be helpful for investigation.
> >> However, I think that we don't log the initial vacuum cost values
> >> depending on the values. For example, if the
> >> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log
> >> the initial values. I think that instead of comparing old and new
> >> values, we can write the log only if
> >> message_level_is_interesting(DEBUG2) is true. That way, we don't need
> >> to acquire the lwlock unnecessarily. And the code looks cleaner to me.
> >> I've attached the patch (use_message_level_is_interesting.patch)
> >
> > Thanks for coming up with the case you thought of with storage param for
> > cost delay = 0. In that case we wouldn't print the message initially and
> > we should fix that.
> >
> > I disagree, however, that we should condition it only on
> > message_level_is_interesting().
>
> I think we should keep the logging frequency as committed, but condition 
> taking
> the lock on message_level_is_interesting().
>
> > Actually, outside of printing initial values when the autovacuum worker
> > first starts (before vacuuming all tables), I don't think we should log
> > these values except when they are being updated. Autovacuum workers
> > could vacuum tons of small tables and having this print out at least
> > once per table (which I know is how it is on master) would be
> > distracting. Also, you could be reloading the config to update some
> > other GUCs and be oblivious to an ongoing autovacuum and get these
> > messages printed out, which I would also find distracting.
> >
> > You will have to stare very hard at the logs to tell if your changes to
> > vacuum cost delay and limit took effect when you reload config. I think
> > with our changes to update the values more often, we should take the
> > opportunity to make this logging more useful by making it happen only
> > when the values are changed.
> >

For debugging purposes, I think it could also be important information
that the cost values are not changed. Personally, I prefer to log the
current state rather than deciding for ourselves which events are
important. If always logging these values in DEBUG2 had been
distracting, we might want to lower it to DEBUG3.

> > I would be open to elevating the log level to DEBUG1 for logging only
> > updates and, perhaps, having an option if you set log level to DEBUG2,
> > for example, to always log these values in VacuumUpdateCosts().

I'm not really sure it's a good idea to change the log messages and
events depending on elevel. Do you know we have any precedents ?

> >
> > I'd even argue that, potentially, having the cost-delay related
> > parameters printed at the beginning of vacuuming could be interesting to
> > regular VACUUM as well (even though it doesn't benefit from config
> > reload while in progress).
> >
> > To fix the issue you mentioned and ensure the logging is printed when
> > autovacuum workers start up before vacuuming tables, we could either
> > initialize vacuum_cost_delay and vacuum_cost_limit to something invalid
> > that will always be different than what they are set to in
> > VacuumUpdateCosts() (not sure if this poses a problem for VACUUM using
> > these values since they are set to the defaults for VACUUM). Or, we
> > could duplicate this logging message in do_autovacuum().
>
> Duplicating logging, maybe with a slightly tailored message, seem the least
> bad option.
>
> > Finally, one other point about message_level_is_interesting(). I liked
> > the idea of using it a lot, since log level DEBUG2 will not be the
> > common case. I thought of it but hesitated because all other users of
> > message_level_is_interesting() are avoiding some memory allocation or
> > string copying -- not avoiding take a lock. Making this conditioned on
> > log level made me a bit uncomfortable. I can't think of a situation when
> > it would be a problem, but it felt a bit off.
>
> Considering how uncommon DEBUG2 will be in production, I think conditioning
> taking a lock on it makes sense.

The comment of message_level_is_interesting() says:

 * This is useful to short-circuit any expensive preparatory work that
 * might be needed for a logging message.

Which can apply to taking a lwlock, I think.

>
> >> Also, while testing the autovacuum delay with relopt
> >> autovacuum_vacuum_cost_delay = 0, I realized that even if we set
> >> autovacuum_vacuum_cost_delay = 0 to a table, wi_dob

Re: running logical replication as the subscription owner

[Robert Haas]

On Mon, Apr 10, 2023 at 10:09 PM Shinoda, Noriyoshi (PN Japan FSIP)
 wrote:
> Hi hackers,
> Thank you for developing a great feature.
> The following commit added a column to the pg_subscription catalog.
>  
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6
>  
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6
>
> I found that the documentation of the pg_subscription catalog was missing an 
> explanation of the columns subrunasowner and subpasswordrequired, so I 
> attached a patch. Please fix the patch if you have a better explanation.

Thank you. Committed.

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

Re: ERROR messages in VACUUM's PARALLEL option

[Melanie Plageman]

On Tue, Apr 11, 2023 at 4:00 AM David Rowley  wrote:
>
> Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> BUFFER_USAGE_LIMIT option.
>
> 1) buffer_usage_limit in the ERROR messages should be consistently in 
> uppercase.

I did notice that all the other VACUUM options don't do this:

postgres=# vacuum (process_toast 'boom') foo;
ERROR:  process_toast requires a Boolean value
postgres=# vacuum (full 2) foo;
ERROR:  full requires a Boolean value
postgres=# vacuum (verbose 2) foo;
ERROR:  verbose requires a Boolean value

Though, I actually prefer uppercase. They are all documented in
uppercase, so I don't see why they would all be lowercase in the error
messages. Additionally, for BUFFER_USAGE_LIMIT, I find the uppercase
helpful to differentiate it from the GUC vacuum_buffer_usage_limit.

> 2) defGetString() already checks for opt->args == NULL and raises an
> ERROR when it is.
>
> I suspect that Melanie might have followed the lead of the PARALLEL
> option when she was working on adding the BUFFER_USAGE_LIMIT patch.

Yes, I mostly imitated parallel since it was the other VACUUM option
with a valid range (as opposed to a boolean or enum of acceptable
values).

> What I'm wondering now is:
>
> a) Is it worth changing this for the PARALLEL option too? and;
> b) I see we lose the parse position indicator, and;

I'm not too worried about the parse position indicator, as we don't get
it for the majority of the errors about valid values. And, as you say
later in your email, the VACUUM options are pretty simple so it should
be easy for the user to figure out without a parse position indicator.

postgres=# vacuum (SKIP_LOCKED 2) foo;
ERROR:  skip_locked requires a Boolean value

While trying different combinations, I noticed that defGetInt32 produces
a somewhat unsatisfactory error message when I provide an argument which
would overflow an int32

postgres=# vacuum (parallel 33) foo;
ERROR:  parallel requires an integer value

In defGetInt32(), the def->arg nodeTag is T_Float, which is why we get
this error message. Perhaps it is worth checking somewhere in the stack
for integer overflow (instead of assuming it is a float) and giving a
more helpful message like the one from parse_int()?

if (val > INT_MAX || val < INT_MIN)
{
if (hintmsg)
*hintmsg = gettext_noop("Value exceeds integer range.");
return false;
}

Probably not a trivial task and not one for 16, however.

I will say that I prefer the new error message introduced by this
patch's usage of defGetInt32() when an argument is not specified to the
previous error message.

postgres=# vacuum (parallel) foo;
ERROR:  parallel requires an integer value

On Tue, Apr 11, 2023 at 9:58 AM Tom Lane  wrote:
>
> David Rowley  writes:
> > Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> > BUFFER_USAGE_LIMIT option.
>
> > 1) buffer_usage_limit in the ERROR messages should be consistently in 
> > uppercase.
>
> FWIW, I think this is exactly backward, and so is whatever code you
> based this on.  Our usual habit is to write GUC names and suchlike
> in lower case in error messages.  Add double quotes if you feel you
> want to set them off from the surrounding text.  Here's a typical
> example of longstanding style:
>
> regression=# set max_parallel_workers_per_gather to -1;
> ERROR:  -1 is outside the valid range for parameter 
> "max_parallel_workers_per_gather" (0 .. 1024)

It seems it also is true for VACUUM options currently, but you didn't
mention command options. Do you also feel these should be lowercase?

- Melanie

[BUG #17888] Incorrect memory access in gist__int_ops for an input array with many elements

[Ankit Kumar Pandey]

Hi,

I was looking at this issue: 
https://www.postgresql.org/message-id/flat/17888-f72930e6b5ce8c14%40postgresql.org


pfree call on contrib/intarray/_int_gist.c:345

```

    if (in != (ArrayType *) DatumGetPointer(entry->key))
    pfree(in);

```

leads to BogusFree function call and server crash.

This is when we use array size of 1001.

If we increase array size to 3001, we get index size error,

NOTICE:  input array is too big (199 maximum allowed, 3001 current), use 
gist__intbig_ops opclass instead

ERROR:  index row requires 12040 bytes, maximum size is 8191

When array size is 1001 is concerned, we raise elog about input array is 
too big, multiple of times. Wouldn't it make more sense to error out 
instead of proceeding further and getting bogus pointer errors messages 
(as we do when size is 3001)?


Changing elog to ereport makes behavior consistent (between array size 
of 1001 vs 3001), so I have


attached a patch for that.

It errors out like this:

ERROR:  input array is too big (199 maximum allowed, 1001 current), use 
gist__intbig_ops opclass instead


This is same error which was raised as notice earlier.

Let me know if it makes sense.


Also, comments on BogusFree mentions `As a possible
aid in debugging, we report the header word along with the pointer
address`. How can we interpret useful debugging information from this?

`pfree called with invalid pointer 0x7ff1706d0030 (header 
0x4fc80001)`



Regards,

Ankit
diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index 7a48ce624d..79c81127f6 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -181,8 +181,10 @@ g_int_compress(PG_FUNCTION_ARGS)
 		PREPAREARR(r);
 
 		if (ARRNELEMS(r) >= 2 * num_ranges)
-			elog(NOTICE, "input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead",
- 2 * num_ranges - 1, ARRNELEMS(r));
+			ereport(ERROR,
+(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead",
+ 2 * num_ranges - 1, ARRNELEMS(r;
 
 		retval = palloc(sizeof(GISTENTRY));
 		gistentryinit(*retval, PointerGetDatum(r),

Re: When to drop src/tools/msvc support

[Andres Freund]

Hi,

On 2023-04-11 09:05:31 +0100, Dave Page wrote:
> Probably my main concern is that the Meson build can use the same version
> of the VC++ compiler that we use (v14), which is carefully matched for
> compatibility with all the various components, just in case anything passes
> CRT pointers around.

FWIW, Independent of meson, I don't think support for VC 2015 in postgres is
long for the world. Later versions of msvc have increased the C standard
compliance a fair bit... It's also a few years out of normal support.

I've not tested building with 2015, but I don't know of anything that should
prevent building with meson with it.  I am fairly sure that it can't build
tab-complete.c, but you're presumably not building with tab completion support
anyway?

Greetings,

Andres Freund

Re: Should we remove vacuum_defer_cleanup_age?

[Justin Pryzby]

On Wed, Mar 22, 2023 at 10:00:48AM -0700, Andres Freund wrote:
> I don't know whether others think we should apply it this release, given the
> "late submission", but I tend to think it's not worth caring the complication
> of vacuum_defer_cleanup_age forward.

I don't see any utility in waiting; it just makes the process of
removing it take longer for no reason.

As long as it's done before the betas, it seems completely reasonable to
remove it for v16. 

-- 
Justin

Re: When to drop src/tools/msvc support

[Andres Freund]

Hi,

On 2023-04-11 10:44:09 -0400, Jonathan S. Katz wrote:
> On 4/11/23 10:12 AM, Tom Lane wrote:
> > "Jonathan S. Katz"  writes:
> > > On 4/11/23 9:49 AM, Tom Lane wrote:
> > > > Sadly, I think we really have to ship both build systems in v16.
> > 
> > > But maybe we can make it clear in the release notes + docs that this is
> > > slated for deprecation and will be removed from v17? That way we can say
> > > "we provided ample warning to move to the new build system."
> > 
> > Yes, we absolutely should do that, as already discussed upthread.
> 
> Ah yes, I saw Andres' notep[1] yesterday and had already forgotten. +1 on
> that plan.

Here's a draft docs change.

I added the  in two places in install-windows.sgml so it's visible
on both the generated pages in the chunked output. That does mean it's visible
twice nearby in the single-page output, but I don't think that's commonly
used.

Greetings,

Andres Freund
>From 11b7229f499917fb1902ce226a31c5e2e7463390 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 11 Apr 2023 10:05:18 -0700
Subject: [PATCH v1] docs: Building with src/tools/msvc is deprecated

I added the  in two places in install-windows.sgml so it's visible
on both the generated pages in the chunked output.
---
 doc/src/sgml/install-windows.sgml | 16 
 doc/src/sgml/installation.sgml| 11 ---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 2db44db2fd9..27a5889d733 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -8,6 +8,14 @@
   on Windows
  
 
+ 
+  
+   Building with this infrastructure is deprecated. It
+   will be removed in PostgreSQL 17. Consider
+   building with meson instead.
+  
+ 
+
  
   It is recommended that most users download the binary distribution for
   Windows, available as a graphical installer package
@@ -62,6 +70,14 @@
   Building with Visual C++ or the
   Microsoft Windows SDK
 
+ 
+  
+   Building with this infrastructure is deprecated. It
+   will be removed in PostgreSQL 17. Consider
+   building with meson instead.
+  
+ 
+
  
   PostgreSQL can be built using the Visual C++ compiler suite from Microsoft.
   These compilers can be either from Visual Studio,
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index f451204854c..b9d3fdeafdb 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -26,9 +26,14 @@ documentation.  See standalone-profile.xsl for details.
 
  
   If you are building PostgreSQL for Microsoft
-  Windows, read this chapter if you intend to build with MinGW or Cygwin;
-  but if you intend to build with Microsoft's Visual
-  C++, see  instead.
+  Windows, read  if you intend to build
+  with make and autoconf (using MinGW or Cygwin build environments); read
+   if you intend to build with
+  meson (using Microsoft Visual
+  C++, MinGW or Cygwin build environments); if you intend to
+  build with Microsoft's Visual C++ using the
+  deprecated perl scripts, see  instead.
  
 
  
-- 
2.38.0

Re: Memory leak from ExecutorState context?

[Jehan-Guillaume de Rorthais]

On Sat, 8 Apr 2023 02:01:19 +0200
Jehan-Guillaume de Rorthais  wrote:

> On Fri, 31 Mar 2023 14:06:11 +0200
> Jehan-Guillaume de Rorthais  wrote:
> 
>  [...]  
> 
> After rebasing Tomas' memory balancing patch, I did some memory measures
> to answer some of my questions. Please, find in attachment the resulting
> charts "HJ-HEAD.png" and "balancing-v3.png" to compare memory consumption
> between HEAD and Tomas' patch. They shows an alternance of numbers
> before/after calling ExecHashIncreaseNumBatches (see the debug patch). I
> didn't try to find the exact last total peak of memory consumption during the
> join phase and before all the BufFiles are destroyed. So the last number
> might be underestimated.

I did some more analysis about the total memory consumption in filecxt of HEAD,
v3 and v4 patches. My previous debug numbers only prints memory metrics during
batch increments or hash table destruction. That means:

* for HEAD: we miss the batches consumed during the outer scan
* for v3: adds twice nbatch in spaceUsed, which is a rough estimation
* for v4: batches are tracked in spaceUsed, so they are reflected in spacePeak

Using a breakpoint in ExecHashJoinSaveTuple to print "filecxt->mem_allocated"
from there, here are the maximum allocated memory for bufFile context for each
branch:

batches   max bufFiles   total  spaceAllowed rise
  HEAD16384  199966960  ~194MB
  v3   4096   65419456   ~78MB
  v4(*3)   2048   3427328048MB  nbatch*sizeof(PGAlignedBlock)*3
  v4(*4)   1024   17170160  60.6MB  nbatch*sizeof(PGAlignedBlock)*4
  v4(*5)   2048   34273280  42.5MB  nbatch*sizeof(PGAlignedBlock)*5

It seems account for bufFile in spaceUsed allows a better memory balancing and
management. The precise factor to rise spaceAllowed is yet to be defined. *3 or
*4 looks good, but this is based on a single artificial test case.

Also, note that HEAD is currently reporting ~4MB of memory usage. This is by
far wrong with the reality. So even if we don't commit the balancing memory
patch in v16, maybe we could account for filecxt in spaceUsed as a bugfix?

Regards,

Re: Documentation for building with meson

[Andres Freund]

Hi,

On 2023-03-28 12:27:26 -0700, samay sharma wrote:
> Subject: [PATCH v9 1/5] Make minor additions and corrections to meson docs
> 
> This commit makes a few corrections to the meson docs
> and adds a few instructions and links for better clarity.
> ---
>  doc/src/sgml/installation.sgml | 24 +++-
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
> index 70ab5b77d8..e3b9b6c0d0 100644
> --- a/doc/src/sgml/installation.sgml
> +++ b/doc/src/sgml/installation.sgml
> @@ -2057,8 +2057,7 @@ meson setup build -Dssl=openssl
>  
>  meson configure -Dcassert=true
>  
> -meson configure's commonly used command-line options
> -are explained in .
> +Commonly used build options for meson configure (and 
> meson setup) are explained in  linkend="meson-options"/>.
> 
>
>  
> @@ -2078,6 +2077,13 @@ ninja
>  processes used with the command line argument -j.
> 
>  
> +   
> +If you want to build the docs, you can type:
> +
> +ninja docs
> +
> +   

"type" sounds a bit too, IDK, process oriented. "To build the docs, use"?


> Subject: [PATCH v9 2/5] Add data layout options sub-section in installation
>  docs
> 
> This commit separates out blocksize, segsize and wal_blocksize
> options into a separate Data layout options sub-section in both
> the make and meson docs. They were earlier in a miscellaneous
> section which included several unrelated options. This change
> also helps reduce the repetition of the warnings that changing
> these parameters breaks on-disk compatibility.

Makes sense. I'm planning to apply this unless Peter or somebody else has
further feedback.


> From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001
> From: Samay Sharma 
> Date: Mon, 13 Feb 2023 16:23:52 -0800
> Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation from
>  source docs
> 
> Currently, several meson setup options are listed in anti-features.
> However, they are similar to most other options in the postgres
> features list as they are 'auto' features themselves. Also, other
> options are likely better suited to the developer options section.
> This commit, therefore, moves the options listed in the anti-features
> section into other sections and removes that section.
> 
> For consistency, this reorganization has been done on the make section
> of the docs as well.

Makes sense to me. "Anti-Features" is confusing as a name to start with.

Greetings,

Andres Freund

Re: When to drop src/tools/msvc support

[Tom Lane]

Andres Freund  writes:
> Here's a draft docs change.

> I added the  in two places in install-windows.sgml so it's visible
> on both the generated pages in the chunked output. That does mean it's visible
> twice nearby in the single-page output, but I don't think that's commonly
> used.

I don't agree with placing that first hunk before the para that tells
people to use a binary distribution, as it's completely irrelevant
if they take that advice.  I'm not really sure we need it at all,
because quite a bit of the text right after that is not specific to using
the src/tools/msvc scripts either.  I think the  under "Building
with Visual C++ ..." is sufficient.

The other two changes look fine.

regards, tom lane

Re: Show various offset arrays for heap WAL records

[Peter Geoghegan]

On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman
 wrote:
> static void
> infobits_desc(StringInfo buf, uint8 infobits, const char *keyname)
> {
> appendStringInfo(buf, "%s: [", keyname);
>
> Why can we assume that there will be no space at the end here?

I don't think that anybody is going to try that, but if they do then
the assertion will fail reliably.

> I know we need to be able to avoid doing the comma overwriting if no
> flags were set. In general, we expect record description elements to
> prepend themselves with commas and spaces, but these infobits, for
> example, use a trailing comma and space. If someone adds a description
> element with a trailing space, they will trip this assert. We should at
> least add a comment explaining this assertion so someone knows what to
> do if they trip it.

The invariant here is roughly: caller's keyname argument cannot have
trailing spaces or punctuation characters. It looks like it would be
inconvenient to write a precise assertion for that, but it doesn't
feel particularly necessary, given that this is just a static helper
function.

> Otherwise, we can return early if no flags are set. That will probably
> make for slightly messier code since we would still have to construct
> the empty list.

I prefer to keep this as simple as possible for now.

> Also you didn't add the same assert to truncate_flags_desc().

That's because truncate_flags_desc doesn't have a "keyname" argument.
Though it does have an assertion at the end that is almost equivalent:
the "Assert(buf->data[buf->len - 2] == ',') assertion (a matching
assertion appears at the end of infobits_desc).

> Not the fault of this patch, but I also noticed that heap UPDATE and
> HOT_UPDATE records have xmax twice and don't differentiate between new
> and old. I think that was probably a mistake.
>
> description  | off: 119, xmax: 1105, flags: 0x00, old_infobits:
> [], new off: 100, xmax 0

That doesn't seem great to me either. I don't like this ambiguity,
because it seems like it makes the description hard to parse in a way
that flies in the face of what we're trying to do here, in general.
So it seems like it might be worth fixing now, in the scope of this
patch.

> Also not the fault of this patch, but looking at the output while using
> this, I realized truncate record type has a stringified version of its
> flags while other record types, like update, don't. Do you think this
> makes sense? Perhaps not something we can change now, though...

You don't have to look at the truncate record type (which is a
relatively obscure and unimportant record type) to see these kinds of
inconsistencies. You can see the same thing with HEAP_UPDATE and
HEAP_HOT_UPDATE, which have stringified constants for infomask bits,
but not for the xl_heap_update.flags status bits.

I don't see any principled reason why such an inconsistency should
exist -- and we're talking about a pretty glaring inconsistency here.
On the other hand I don't think that we're obligated to do anything
about it for 16.

> Also not the fault of this patch, but I noticed that leaftopparent is
> often InvalidBlockNumber--which shows up as 4294967295. I wonder if
> anyone would be confused by this. Maybe devs know that this value is
> InvalidBlockNumber. In the future, perhaps we should interpolate the
> string "InvalidBlockNumber"?
>
> description | left: 436, right: 389, level: 0, safexid: 0:1091,
> leafleft: 436, leafright: 389, leaftopparent: 4294967295

In my personal opinion (this is a totally subjective question), the
current approach here is okay because (on its own) "leaftopparent:
4294967295" isn't any more or less meaningful than "leaftopparent:
InvalidBlockNumber". It's not as if the REDO routine actually relies
on the value ever being InvalidBlockNumber at all (except in an
assertion).

Plus it's easier to parse as-is. That's what swings it for me, in fact
(as with the "two xmax fields in update records" question).

This is the kind of question that tends to lead to bikeshedding. The
guidelines should avoid taking a firm position on these more
subjective questions, where we must make a subjective trade-off.
Especially a trade-off around how faithfully we represent the physical
WAL record versus readability (whatever "readability" means). I
pondered a similar trade-off in comments added to delvacuum_desc. That
contributed to my feeling that this is best left up to individual rmgr
desc routines.

--
Peter Geoghegan

Re: Fix the miss consideration of tuple_fraction during add_paths_to_append_rel

[Andy Fan]

On Mon, Apr 10, 2023 at 9:56 PM Zhang Mingli  wrote:

>
> There is spare indent at else if.
>
> - if (childrel->pathlist != NIL &&
> + if (cheapest_startup_path && cheapest_startup_path->param_info == NULL)
> + accumulate_append_subpath(cheapest_startup_path,
> +   &subpaths, NULL);
> + else if (childrel->pathlist != NIL &&
>   childrel->cheapest_total_path->param_info == NULL)
>   accumulate_append_subpath(childrel->cheapest_total_path,
> &subpaths, NULL);
>
> Could we also consider tuple_fraction in partial_pathlist for  parallel
> append?
>
>
Thanks for the suggestion,  the v2 has fixed the indent issue and I did
something about parallel append.  Besides that,  I restrict the changes
happens under bms_equal(rel->relids, root->all_query_rels), which may
make this patch safer.

I have added this into https://commitfest.postgresql.org/43/4270/

-- 
Best Regards
Andy Fan


v2-0001-Considering-root-tuple_fraction-during-create_app.patch
Description: Binary data

Re: Direct I/O

[Andres Freund]

Hi,

On 2023-04-09 16:40:54 -0700, Noah Misch wrote:
> On Sun, Apr 09, 2023 at 02:45:16PM -0700, Andres Freund wrote:
> > It's not *just* that scenario. With a few concurrent connections you can get
> > into problematic territory even with halfway reasonable shared buffers.
>
> I am not familiar with such cases.  You could get there with 64MB shared
> buffers and 256 simultaneous commits of new-refilenode-creating transactions,
> but I'd still file that under going out of one's way to use tiny shared
> buffers relative to the write activity.  What combination did you envision?

I'd not say it's common, but it's less crazy than running with 128kB of s_b...

There's also the issue that log_newpage_range() is used in number of places
where we could have a lot of pre-existing buffer pins. So pinning another 64
buffers could tip us over.

Greetings,

Andres Freund

Re: When to drop src/tools/msvc support

[Andres Freund]

Hi,

On 2023-04-11 13:30:15 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Here's a draft docs change.
> 
> > I added the  in two places in install-windows.sgml so it's visible
> > on both the generated pages in the chunked output. That does mean it's 
> > visible
> > twice nearby in the single-page output, but I don't think that's commonly
> > used.
> 
> I don't agree with placing that first hunk before the para that tells
> people to use a binary distribution, as it's completely irrelevant
> if they take that advice.

Fair point.


> I'm not really sure we need it at all, because quite a bit of the text right
> after that is not specific to using the src/tools/msvc scripts either.  I
> think the  under "Building with Visual
> C++ ..." is sufficient.

It seemed nicer to have it on all the "Installation from Source Code on Windows"
pages, but...

Except that we're planning to remove it anyway, the structure of the docs here
seems a bit off...

Greetings,

Andres Freund

Re: Should we remove vacuum_defer_cleanup_age?

[Andres Freund]

Hi,

On 2023-04-11 11:33:01 -0500, Justin Pryzby wrote:
> On Wed, Mar 22, 2023 at 10:00:48AM -0700, Andres Freund wrote:
> > I don't know whether others think we should apply it this release, given the
> > "late submission", but I tend to think it's not worth caring the 
> > complication
> > of vacuum_defer_cleanup_age forward.
>
> I don't see any utility in waiting; it just makes the process of
> removing it take longer for no reason.
>
> As long as it's done before the betas, it seems completely reasonable to
> remove it for v16.

Added the RMT.

We really should have a rmt@pg.o alias...

Updated patch attached. I think we should either apply something like that
patch, or at least add a  to the docs.

Greetings,

Andres Freund
>From c51c9e450b1b1342c0f6ff32e7e360677ccbc2c6 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 11 Apr 2023 10:21:36 -0700
Subject: [PATCH v2] Remove vacuum_defer_cleanup_age

This commit removes TransactionIdRetreatSafely(), as there are no users
anymore. There might be potential future users, hence noting that here.

Reviewed-by: Daniel Gustafsson 
Reviewed-by: Justin Pryzby 
Discussion: https://postgr.es/m/20230317230930.nhsgk3qfk7f4a...@awork3.anarazel.de
---
 src/include/storage/standby.h |   1 -
 src/backend/storage/ipc/procarray.c   | 105 ++
 src/backend/storage/ipc/standby.c |   1 -
 src/backend/utils/misc/guc_tables.c   |   9 --
 src/backend/utils/misc/postgresql.conf.sample |   1 -
 src/bin/pg_upgrade/server.c   |   5 +-
 doc/src/sgml/config.sgml  |  35 --
 doc/src/sgml/high-availability.sgml   |  28 +
 8 files changed, 15 insertions(+), 170 deletions(-)

diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 41f4dc372e6..e8f50569491 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -21,7 +21,6 @@
 #include "storage/standbydefs.h"
 
 /* User-settable GUC parameters */
-extern PGDLLIMPORT int vacuum_defer_cleanup_age;
 extern PGDLLIMPORT int max_standby_archive_delay;
 extern PGDLLIMPORT int max_standby_streaming_delay;
 extern PGDLLIMPORT bool log_recovery_conflict_waits;
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ea91ce355f2..106b184a3e6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -367,9 +367,6 @@ static inline void ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId l
 static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
 static void MaintainLatestCompletedXid(TransactionId latestXid);
 static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
-static void TransactionIdRetreatSafely(TransactionId *xid,
-	   int retreat_by,
-	   FullTransactionId rel);
 
 static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
   TransactionId xid);
@@ -1709,10 +1706,7 @@ TransactionIdIsActive(TransactionId xid)
  * do about that --- data is only protected if the walsender runs continuously
  * while queries are executed on the standby.  (The Hot Standby code deals
  * with such cases by failing standby queries that needed to access
- * already-removed data, so there's no integrity bug.)  The computed values
- * are also adjusted with vacuum_defer_cleanup_age, so increasing that setting
- * on the fly is another easy way to make horizons move backwards, with no
- * consequences for data integrity.
+ * already-removed data, so there's no integrity bug.)
  *
  * Note: the approximate horizons (see definition of GlobalVisState) are
  * updated by the computations done here. That's currently required for
@@ -1877,50 +1871,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
 		/* temp relations cannot be accessed in recovery */
 	}
-	else
-	{
-		/*
-		 * Compute the cutoff XID by subtracting vacuum_defer_cleanup_age.
-		 *
-		 * vacuum_defer_cleanup_age provides some additional "slop" for the
-		 * benefit of hot standby queries on standby servers.  This is quick
-		 * and dirty, and perhaps not all that useful unless the primary has a
-		 * predictable transaction rate, but it offers some protection when
-		 * there's no walsender connection.  Note that we are assuming
-		 * vacuum_defer_cleanup_age isn't large enough to cause wraparound ---
-		 * so guc.c should limit it to no more than the xidStopLimit threshold
-		 * in varsup.c.  Also note that we intentionally don't apply
-		 * vacuum_defer_cleanup_age on standby servers.
-		 *
-		 * Need to use TransactionIdRetreatSafely() instead of open-coding the
-		 * subtraction, to prevent creating an xid before
-		 * FirstNormalTransactionId.
-		 */
-		Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
-			 h->shared_oldest_nonremovable));
-		Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_non

[PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Aleksander Alekseev]

Hi,

While playing with a new single board computer (VisionFive 2) I
discovered that postgresql:unsafe_tests suite fails like this:

```
--- 
/home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out
2023-04-11 14:58:57.844550612 +
+++ 
/home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out
2023-04-11 17:54:22.999024391 +
@@ -53,6 +53,7 @@
 CREATE ROLE "current_user";
 CREATE ROLE "session_user";
 CREATE ROLE "user";
+ERROR:  role "user" already exists
 RESET client_min_messages;
 CREATE ROLE current_user; -- error
 ERROR:  CURRENT_USER cannot be used as a role name here
@@ -1089,4 +1090,5 @@
 DROP OWNED BY regress_testrol0, "Public", "current_role",
"current_user", regress_testrol1, regress_testrol2, regress_testrolx
CASCADE;
 DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx;
 DROP ROLE "Public", "None", "current_role", "current_user",
"session_user", "user";
+ERROR:  current user cannot be dropped
 DROP ROLE regress_role_haspriv, regress_role_nopriv;
```

This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.

Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_user"? It
is also a keyword but is a less likely choice for the OS user name.

-- 
Best regards,
Aleksander Alekseev


v1-0001-Use-role-name-system_user-instead-of-user-for-uns.patch
Description: Binary data

Issue in postgres_fdw causing unnecessary wait for cancel request reply

[Fujii Masao]

Hi,

When using postgres_fdw, in the event of a local transaction being
aborted while a query is running on a remote server,
postgres_fdw sends a cancel request to the remote server.
However, if PQgetCancel() returned NULL and no cancel request was issued,
I found that postgres_fdw could still wait for the reply to
the cancel request, causing unnecessary wait time with a 30 second timeout.

For example, the following queries can reproduce the issue:


create extension postgres_fdw;
create server loopback foreign data wrapper postgres_fdw options 
(tcp_user_timeout 'a');
create user mapping for public server loopback;
create view t as select 1 as i from pg_sleep(100);
create foreign table ft (i int) server loopback options (table_name 't');
select * from ft;

Press Ctrl-C while running the above SELECT query.


Attached patch fixes this issue. It ensures that postgres_fdw only waits
for a reply if a cancel request is actually issued. Additionally,
it improves PQgetCancel() to set error messages in certain error cases,
such as when out of memory occurs and malloc() fails. Moreover,
it enhances postgres_fdw to report a warning message when PQgetCancel()
returns NULL, explaining the reason for the NULL value.

Thought?

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 26293ade92ce6fa0bbafac4a95f634e485f4aab6 Mon Sep 17 00:00:00 2001
From: Fujii Masao 
Date: Wed, 12 Apr 2023 02:17:56 +0900
Subject: [PATCH v1] postgres_fdw: Fix bug causing unnecessary wait for cancel
 request reply.

In the event of a local transaction being aborted while a query is
running on a remote server in postgres_fdw, the extension sends
a cancel request to the remote server. However, if PQgetCancel()
returned NULL and no cancel request was issued, postgres_fdw
could still wait for the reply to the cancel request, causing unnecessary
wait time with a 30 second timeout.

To fix this issue, this commit ensures that postgres_fdw only waits for
a reply if a cancel request was actually issued. Additionally,
this commit improves PQgetCancel() to set error messages in certain
error cases, such as when out of memory happens and malloc() fails.
Moreover, this commit enhances postgres_fdw to report a warning
message when PQgetCancel() returns NULL, explaining the reason for
the NULL value.
---
 contrib/postgres_fdw/connection.c | 8 
 src/interfaces/libpq/fe-connect.c | 6 ++
 2 files changed, 14 insertions(+)

diff --git a/contrib/postgres_fdw/connection.c 
b/contrib/postgres_fdw/connection.c
index 2969351e9a..e3c79158d8 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1344,6 +1344,14 @@ pgfdw_cancel_query_begin(PGconn *conn)
}
PQfreeCancel(cancel);
}
+   else
+   {
+   ereport(WARNING,
+   (errcode(ERRCODE_CONNECTION_FAILURE),
+errmsg("could not send cancel request: %s",
+   pchomp(PQerrorMessage(conn);
+   return false;
+   }
 
return true;
 }
diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index 40fef0e2c8..eeb4d9a745 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4703,11 +4703,17 @@ PQgetCancel(PGconn *conn)
return NULL;
 
if (conn->sock == PGINVALID_SOCKET)
+   {
+   libpq_append_conn_error(conn, "connection is not open");
return NULL;
+   }
 
cancel = malloc(sizeof(PGcancel));
if (cancel == NULL)
+   {
+   libpq_append_conn_error(conn, "out of memory");
return NULL;
+   }
 
memcpy(&cancel->raddr, &conn->raddr, sizeof(SockAddr));
cancel->be_pid = conn->be_pid;
-- 
2.40.0

Re: [PATCH] Support % wildcard in extension upgrade filenames

[Sandro Santilli]

On Mon, Apr 10, 2023 at 11:09:40PM -0400, Regina Obe wrote:
> > On Mon, Apr 03, 2023 at 09:26:25AM +0700, Yurii Rashkovskii wrote:
> > > I want to chime in on the issue of lower-number releases that are
> > > released after higher-number releases. The way I see this particular
> > > problem is that we always put upgrade SQL files in release "packages,"
> > > and they obviously become static resources.
> > >
> > > While I [intentionally] overlook some details here, what if (as a
> > > convention, for projects where it matters) we shipped extensions with
> > > non-upgrade SQL files only, and upgrades were available as separate
> > > downloads? This way, we're not tying releases themselves to upgrade paths.
> > > This also requires no changes to Postgres.
> > 
> > This is actually something that's on the plate, and we recently added a --
> > disable-extension-upgrades-install configure switch and a 
> > `install-extension-
> > upgrades-from-known-versions` make target in PostGIS to help going in that
> > direction. I guess the ball would now be in the hands of packagers.
> > 
> > > I know this may be a big delivery layout departure for
> > > well-established projects; I also understand that this won't solve the
> > > problem of having to have these files in the first place (though in
> > > many cases, they can be automatically generated once, I suppose, if they 
> > > are
> > trivial).
> > 
> > We will now also be providing a `postgis` script for administration that 
> > among
> > other things will support a `install-extension-upgrades` command to install
> > upgrade paths from specific old versions, but will not hard-code any list of
> > "known" old versions as such a list will easily become outdated.
> > 
> > Note that all upgrade scripts installed by the Makefile target or by the
> > `postgis` scripts will only be empty upgrade paths from the source version 
> > to
> > the fake "ANY" version, as the ANY-- upgrade path will still be the
> > "catch-all" upgrade script.
> 
> Sounds like a user and packaging nightmare to me.
> How is a packager to know which versions a user might have installed?

Isn't this EXACTLY the same problem WE have ? The problem is we cannot
know in advance how many patch-level releases will come out, and we
don't want to hurry into cutting a new release in branches NOT having
a bug which was fixed in a stable branch...

Packager might actually know better in that they could ONLY consider
the packages ever packaged by them.

Hey, best would be having support for wildcard wouldn't it ? 

> I much preferred the idea of just listing all our upgrade targets in the 
> control file.

Again: how would we know all upgrade targets ?

> We need to come up with a convention of how to describe a micro update,
> as it's really a problem with extensions that follow the pattern

I think it's a problem with extensions maintaining stable branches,
as if the history was linear we would possibly need less files
(although at this stage any number bigger than 1 would be too much for me)

--strk;

Re: Show various offset arrays for heap WAL records

[Peter Geoghegan]

On Tue, Apr 11, 2023 at 10:34 AM Peter Geoghegan  wrote:
> > description  | off: 119, xmax: 1105, flags: 0x00, old_infobits:
> > [], new off: 100, xmax 0
>
> That doesn't seem great to me either. I don't like this ambiguity,
> because it seems like it makes the description hard to parse in a way
> that flies in the face of what we're trying to do here, in general.
> So it seems like it might be worth fixing now, in the scope of this
> patch.

Attached revision deals with this by spelling out the names in full
(e.g., "old_xmax" and "new_xmax"). It also reorders the output fields
to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL
record types, on the theory that those should match the physical
record (unless there is a good reason not to, which doesn't apply
here). I also removed some inconsistencies between
xl_heap_lock_updated and xl_heap_lock, since they're very similar
record types.

The revision also adds an extra sentence to the guidelines, since this
seems like something that we're entitled to take a relatively firm
position on. Finally, it also adds a comment about the rules for
infobits_desc callers in header comments for the function, per your
concern about that.

-- 
Peter Geoghegan


v3-0001-Fix-heapdesc-infomask-array-output.patch
Description: Binary data

Assertion being hit during WAL replay

[Tom Lane]

I have seen this failure a couple of times recently while
testing code that caused crashes and restarts:

#2  0x009987e3 in ExceptionalCondition (
conditionName=conditionName@entry=0xb31bc8 "mode == RBM_NORMAL || mode == 
RBM_ZERO_ON_ERROR || mode == RBM_ZERO_AND_LOCK", 
fileName=fileName@entry=0xb31c15 "bufmgr.c", 
lineNumber=lineNumber@entry=892) at assert.c:66
#3  0x00842d73 in ExtendBufferedRelTo (eb=..., 
fork=fork@entry=MAIN_FORKNUM, strategy=strategy@entry=0x0, 
flags=flags@entry=3, extend_to=extend_to@entry=1, 
mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK) at bufmgr.c:891
#4  0x005cc398 in XLogReadBufferExtended (rlocator=..., 
forknum=MAIN_FORKNUM, blkno=0, mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK, 
recent_buffer=) at xlogutils.c:527
#5  0x005cc697 in XLogReadBufferForRedoExtended (
record=record@entry=0x1183b98, block_id=block_id@entry=0 '\000', 
mode=mode@entry=RBM_NORMAL, get_cleanup_lock=get_cleanup_lock@entry=true, 
buf=buf@entry=0x7ffd98e3ea94) at xlogutils.c:391
#6  0x0055df59 in heap_xlog_prune (record=0x1183b98) at heapam.c:8779
#7  heap2_redo (record=0x1183b98) at heapam.c:10015
#8  0x005ca430 in ApplyWalRecord (replayTLI=, 
record=0x7f8f7afbcb60, xlogreader=)
at ../../../../src/include/access/xlog_internal.h:379

It's not clear to me whether this Assert is wrong, or
XLogReadBufferForRedoExtended shouldn't be using
RBM_ZERO_AND_CLEANUP_LOCK, or the Assert is correctly protecting an
unimplemented case in ExtendBufferedRelTo that we now need to implement.

In any case, I'm pretty sure Andres broke it in 26158b852, because
I hadn't seen it before this weekend.

regards, tom lane

Re: When to drop src/tools/msvc support

[Tom Lane]

Andres Freund  writes:
> Except that we're planning to remove it anyway, the structure of the docs here
> seems a bit off...

Indeed.  We'll have to migrate some of that info elsewhere when the
time comes.

regards, tom lane

Re: fairywren exiting in ecpg

[Andres Freund]

Hi,

On 2023-04-11 07:10:20 -0400, Andrew Dunstan wrote:
> The error hasn't been seen since I set this about a week ago.

This issue really bothers me, but I am at my wits end how to debug it, given
that we get a segfault only if we *disable* getting crash reports / core dumps
in some form. There's no debug printout or anything, python just exits with an
error code indicating an access violation.

Greetings,

Andres Freund

Re: refactoring relation extension and BufferAlloc(), faster COPY

[Alexander Lakhin]

Hi Andres,

07.04.2023 11:39, Andres Freund wrote:

Hi,

On 2023-04-06 18:15:14 -0700, Andres Freund wrote:

I think it might be worth having a C test for some of the bufmgr.c API. Things
like testing that retrying a failed relation extension works the second time
round.

A few hours after this I hit a stupid copy-pasto (21d7c05a5cf) that would
hopefully have been uncovered by such a test...


A few days later I've found a new defect introduced with 31966b151.
The following script:
echo "
CREATE TABLE tbl(id int);
INSERT INTO tbl(id) SELECT i FROM generate_series(1, 1000) i;
DELETE FROM tbl;
CHECKPOINT;
" | psql -q

sleep 2

grep -C2 'automatic vacuum of table ".*.tbl"' server.log

tf=$(psql -Aqt -c "SELECT format('%s/%s', pg_database.oid, relfilenode) FROM pg_database, pg_class WHERE datname = 
current_database() AND relname = 'tbl'")

ls -l "$PGDB/base/$tf"

pg_ctl -D $PGDB stop -m immediate
pg_ctl -D $PGDB -l server.log start

with the autovacuum enabled as follows:
autovacuum = on
log_autovacuum_min_duration = 0
autovacuum_naptime = 1

gives:
2023-04-11 20:56:56.261 MSK [675708] LOG:  checkpoint starting: immediate force 
wait
2023-04-11 20:56:56.324 MSK [675708] LOG:  checkpoint complete: wrote 900 buffers (5.5%); 0 WAL file(s) added, 0 
removed, 0 recycled; write=0.016 s, sync=0.034 s, total=0.063 s; sync files=252, longest=0.017 s, average=0.001 s; 
distance=4162 kB, estimate=4162 kB; lsn=0/1898588, redo lsn=0/1898550

2023-04-11 20:56:57.558 MSK [676060] LOG:  automatic vacuum of table 
"testdb.public.tbl": index scans: 0
    pages: 5 removed, 0 remain, 5 scanned (100.00% of total)
    tuples: 1000 removed, 0 remain, 0 are dead but not yet removable
-rw--- 1 law law 0 апр 11 20:56 .../tmpdb/base/16384/16385
waiting for server to shut down done
server stopped
waiting for server to start stopped waiting
pg_ctl: could not start server
Examine the log output.

server stops with the following stack trace:
Core was generated by `postgres: startup recovering 00010001
 '.
Program terminated with signal SIGABRT, Aborted.

warning: Section `.reg-xstate/790626' in core file too small.
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140209454906240) 
at ./nptl/pthread_kill.c:44
44  ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140209454906240) 
at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140209454906240) at 
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140209454906240, signo=signo@entry=6) at 
./nptl/pthread_kill.c:89
#3  0x7f850ec53476 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26
#4  0x7f850ec397f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x557950889c0b in ExceptionalCondition (
    conditionName=0x557950a67680 "mode == RBM_NORMAL || mode == RBM_ZERO_AND_LOCK || 
mode == RBM_ZERO_ON_ERROR",
    fileName=0x557950a673e8 "bufmgr.c", lineNumber=1008) at assert.c:66
#6  0x55795064f2d0 in ReadBuffer_common (smgr=0x557952739f38, 
relpersistence=112 'p', forkNum=MAIN_FORKNUM,
    blockNum=4294967295, mode=RBM_ZERO_AND_CLEANUP_LOCK, strategy=0x0, 
hit=0x7fff22dd648f) at bufmgr.c:1008
#7  0x55795064ebe7 in ReadBufferWithoutRelcache (rlocator=..., 
forkNum=MAIN_FORKNUM, blockNum=4294967295,
    mode=RBM_ZERO_AND_CLEANUP_LOCK, strategy=0x0, permanent=true) at 
bufmgr.c:800
#8  0x55795021c0fa in XLogReadBufferExtended (rlocator=..., 
forknum=MAIN_FORKNUM, blkno=0,
    mode=RBM_ZERO_AND_CLEANUP_LOCK, recent_buffer=0) at xlogutils.c:536
#9  0x55795021bd92 in XLogReadBufferForRedoExtended (record=0x5579526c4998, 
block_id=0 '\000', mode=RBM_NORMAL,
    get_cleanup_lock=true, buf=0x7fff22dd6598) at xlogutils.c:391
#10 0x5579501783b1 in heap_xlog_prune (record=0x5579526c4998) at 
heapam.c:8726
#11 0x55795017b7db in heap2_redo (record=0x5579526c4998) at heapam.c:9960
#12 0x557950215b34 in ApplyWalRecord (xlogreader=0x5579526c4998, 
record=0x7f85053d0120, replayTLI=0x7fff22dd6720)
    at xlogrecovery.c:1915
#13 0x557950215611 in PerformWalRecovery () at xlogrecovery.c:1746
#14 0x557950201ce3 in StartupXLOG () at xlog.c:5433
#15 0x5579505cb6d2 in StartupProcessMain () at startup.c:267
#16 0x5579505be9f7 in AuxiliaryProcessMain (auxtype=StartupProcess) at 
auxprocess.c:141
#17 0x5579505ca2b5 in StartChildProcess (type=StartupProcess) at 
postmaster.c:5369
#18 0x5579505c5224 in PostmasterMain (argc=3, argv=0x5579526c3e70) at 
postmaster.c:1455
#19 0x55795047a97d in main (argc=3, argv=0x5579526c3e70) at main.c:200

As I can see, autovacuum removes pages from the table file, and this causes
the crash while replaying the record:
rmgr: Heap2   len (rec/tot): 60/   988, tx:  0, lsn: 0/01898600, prev 0/01898588, desc: PRUNE 
snapshotConflictHorizon 732 nredirected 0 ndead 226, blkref #0: rel 1663/16384/16385 blk 0 FPW


Best regards,
Al

Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Andrew Dunstan]

On 2023-04-11 Tu 14:25, Aleksander Alekseev wrote:

Hi,

While playing with a new single board computer (VisionFive 2) I
discovered that postgresql:unsafe_tests suite fails like this:

```
--- 
/home/user/projects/postgresql/src/test/modules/unsafe_tests/expected/rolenames.out
2023-04-11 14:58:57.844550612 +
+++ 
/home/user/projects/postgresql/build/testrun/unsafe_tests/regress/results/rolenames.out
 2023-04-11 17:54:22.999024391 +
@@ -53,6 +53,7 @@
  CREATE ROLE "current_user";
  CREATE ROLE "session_user";
  CREATE ROLE "user";
+ERROR:  role "user" already exists
  RESET client_min_messages;
  CREATE ROLE current_user; -- error
  ERROR:  CURRENT_USER cannot be used as a role name here
@@ -1089,4 +1090,5 @@
  DROP OWNED BY regress_testrol0, "Public", "current_role",
"current_user", regress_testrol1, regress_testrol2, regress_testrolx
CASCADE;
  DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx;
  DROP ROLE "Public", "None", "current_role", "current_user",
"session_user", "user";
+ERROR:  current user cannot be dropped
  DROP ROLE regress_role_haspriv, regress_role_nopriv;
```

This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.

Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_user"? It
is also a keyword but is a less likely choice for the OS user name.



I don't think we can protect against all possible user names. Wouldn't 
it be better to run the tests under an OS user with a different name, 
like "marmaduke"? ("user" is a truly terrible default user name).



cheers


andrew


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

Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Aleksander Alekseev]

Hi Andrew,

> I don't think we can protect against all possible user names. Wouldn't it be 
> better to run the tests under an OS user with a different name, like 
> "marmaduke"? ("user" is a truly terrible default user name).

100% agree. The point is not to protect against all possible user
names but merely to reduce the likelihood of the problem. For this
particular test there is no difference which keyword to use for the
test. I realize this is a minor problem, however the fix is trivial
too.

-- 
Best regards,
Aleksander Alekseev

Re: Show various offset arrays for heap WAL records

[Peter Geoghegan]

On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan  wrote:
> Attached revision deals with this by spelling out the names in full
> (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields
> to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL
> record types, on the theory that those should match the physical
> record (unless there is a good reason not to, which doesn't apply
> here).

I just noticed that we don't even show xmax in the case of DELETE
records. Perhaps the original assumption is that it must match the
record's own XID, but that's not true after the MultiXact enhancements
for foreign key locking added to 9.3 (and in any case there is no
reason at all to show xmax in UPDATE but not in DELETE).

Attached revision v4 fixes this, making DELETE, UPDATE, HOT_UPDATE,
LOCK, and LOCK_UPDATED record types consistent with each other in
terms of the key names output by the heap desc routine. The field
order also needed a couple of tweaks for struct consistency (and
cross-record consistency) for v4.

-- 
Peter Geoghegan


v4-0001-Fix-heapdesc-infomask-array-output.patch
Description: Binary data

v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Justin Pryzby]

Yes, $SUBJECT is correct.

On an old centos6 VM which I'd forgotten about and never removed from
monitoring, I noticed that a process had recently crashed...

Maybe this is an issue which was already fixed, but I looked and find no
bug report nor patch about it.  Feel free to dismiss the problem report
if it's not interesting or useful. 

postgres was compiled locally at 4e54d231a.  It'd been running
continuously since September without crashing until a couple weeks ago
(and running nearly-continuously for months before that).

The VM is essentially idle, so maybe that's related to the crash.

TRAP: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC 
^ area->control->handle ^ index)", File: "dsa.c", Line: 1770, PID: 24257)
postgres: telsasoft old_ts ::1(50284) 
authentication(ExceptionalCondition+0x91)[0x991451]
postgres: telsasoft old_ts ::1(50284) authentication[0x9b9f97]
postgres: telsasoft old_ts ::1(50284) 
authentication(dsa_get_address+0x92)[0x9ba192]
postgres: telsasoft old_ts ::1(50284) 
authentication(pgstat_get_entry_ref+0x442)[0x868892]
postgres: telsasoft old_ts ::1(50284) 
authentication(pgstat_prep_pending_entry+0x54)[0x862b14]
postgres: telsasoft old_ts ::1(50284) 
authentication(pgstat_assoc_relation+0x54)[0x866764]
postgres: telsasoft old_ts ::1(50284) authentication(_bt_first+0xb1b)[0x51399b]
postgres: telsasoft old_ts ::1(50284) authentication(btgettuple+0xc2)[0x50e792]
postgres: telsasoft old_ts ::1(50284) 
authentication(index_getnext_tid+0x51)[0x4ff271]
postgres: telsasoft old_ts ::1(50284) 
authentication(index_getnext_slot+0x72)[0x4ff442]
postgres: telsasoft old_ts ::1(50284) 
authentication(systable_getnext+0x132)[0x4fe282]
postgres: telsasoft old_ts ::1(50284) authentication[0x9775cb]
postgres: telsasoft old_ts ::1(50284) 
authentication(SearchCatCache+0x20d)[0x978e6d]
postgres: telsasoft old_ts ::1(50284) 
authentication(GetSysCacheOid+0x30)[0x98c7c0]
postgres: telsasoft old_ts ::1(50284) 
authentication(get_role_oid+0x2d)[0x86b1ad]
postgres: telsasoft old_ts ::1(50284) 
authentication(hba_getauthmethod+0x22)[0x6f5592]
postgres: telsasoft old_ts ::1(50284) 
authentication(ClientAuthentication+0x39)[0x6f1f59]
postgres: telsasoft old_ts ::1(50284) 
authentication(InitPostgres+0x8c6)[0x9a33d6]
postgres: telsasoft old_ts ::1(50284) 
authentication(PostgresMain+0x109)[0x84bb79]
postgres: telsasoft old_ts ::1(50284) 
authentication(PostmasterMain+0x1a6a)[0x7ac1aa]
postgres: telsasoft old_ts ::1(50284) authentication(main+0x461)[0x6fe5a1]
/lib64/libc.so.6(__libc_start_main+0x100)[0x36e041ed20]

(gdb) bt
#0  0x0036e04324f5 in raise () from /lib64/libc.so.6
#1  0x0036e0433cd5 in abort () from /lib64/libc.so.6
#2  0x00991470 in ExceptionalCondition (conditionName=, errorType=, fileName=, 
lineNumber=1770) at assert.c:69
#3  0x009b9f97 in get_segment_by_index (area=0x22818c0, index=) at dsa.c:1769
#4  0x009ba192 in dsa_get_address (area=0x22818c0, dp=1099511703168) at 
dsa.c:953
#5  0x00868892 in pgstat_get_entry_ref (kind=PGSTAT_KIND_RELATION, 
dboid=, objoid=, create=true, 
created_entry=0x0) at pgstat_shmem.c:508
#6  0x00862b14 in pgstat_prep_pending_entry (kind=PGSTAT_KIND_RELATION, 
dboid=0, objoid=2676, created_entry=0x0) at pgstat.c:1067
#7  0x00866764 in pgstat_prep_relation_pending (rel=0x22beba8) at 
pgstat_relation.c:855
#8  pgstat_assoc_relation (rel=0x22beba8) at pgstat_relation.c:138
#9  0x0051399b in _bt_first (scan=0x22eb5c8, dir=ForwardScanDirection) 
at nbtsearch.c:882
#10 0x0050e792 in btgettuple (scan=0x22eb5c8, dir=ForwardScanDirection) 
at nbtree.c:243
#11 0x004ff271 in index_getnext_tid (scan=0x22eb5c8, direction=) at indexam.c:533
#12 0x004ff442 in index_getnext_slot (scan=0x22eb5c8, 
direction=ForwardScanDirection, slot=0x22eb418) at indexam.c:625
#13 0x004fe282 in systable_getnext (sysscan=0x22eb3c0) at genam.c:511
#14 0x009775cb in SearchCatCacheMiss (cache=0x229e280, nkeys=, hashValue=3877703461, hashIndex=5, v1=, 
v2=, v3=0, v4=0) at catcache.c:1364
#15 0x00978e6d in SearchCatCacheInternal (cache=0x229e280, v1=36056248, 
v2=0, v3=0, v4=0) at catcache.c:1295
#16 SearchCatCache (cache=0x229e280, v1=36056248, v2=0, v3=0, v4=0) at 
catcache.c:1149
#17 0x0098c7c0 in GetSysCacheOid (cacheId=10, oidcol=, key1=, key2=, key3=, key4=) at syscache.c:1293
#18 0x0086b1ad in get_role_oid (rolname=0x2262cb8 "telsasoft", 
missing_ok=true) at acl.c:5181
#19 0x006f5592 in check_hba (port=) at hba.c:2100
#20 hba_getauthmethod (port=) at hba.c:2699
#21 0x006f1f59 in ClientAuthentication (port=0x224b5d0) at auth.c:396
#22 0x009a33d6 in PerformAuthentication (in_dbname=0x2268a48 "old_ts", 
dboid=0, username=0x2262cb8 "telsasoft", useroid=0, out_dbname=0x0, 
override_allow_connections=false) at postinit.c:245
#23 InitPostgres (in_dbname=0x2268a48 "old_ts", dboid=0, username=0x2262cb8 
"telsasoft", useroid=0, out_

Re: Assertion being hit during WAL replay

[Andres Freund]

Hi,

On 2023-04-11 14:48:44 -0400, Tom Lane wrote:
> I have seen this failure a couple of times recently while
> testing code that caused crashes and restarts:

Do you have a quick repro recipe?


> #2  0x009987e3 in ExceptionalCondition (
> conditionName=conditionName@entry=0xb31bc8 "mode == RBM_NORMAL || mode == 
> RBM_ZERO_ON_ERROR || mode == RBM_ZERO_AND_LOCK",
> fileName=fileName@entry=0xb31c15 "bufmgr.c",
> lineNumber=lineNumber@entry=892) at assert.c:66
> #3  0x00842d73 in ExtendBufferedRelTo (eb=...,
> fork=fork@entry=MAIN_FORKNUM, strategy=strategy@entry=0x0,
> flags=flags@entry=3, extend_to=extend_to@entry=1,
> mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK) at bufmgr.c:891
> #4  0x005cc398 in XLogReadBufferExtended (rlocator=...,
> forknum=MAIN_FORKNUM, blkno=0, mode=mode@entry=RBM_ZERO_AND_CLEANUP_LOCK,
> recent_buffer=) at xlogutils.c:527
> #5  0x005cc697 in XLogReadBufferForRedoExtended (
> record=record@entry=0x1183b98, block_id=block_id@entry=0 '\000',
> mode=mode@entry=RBM_NORMAL, get_cleanup_lock=get_cleanup_lock@entry=true,
> buf=buf@entry=0x7ffd98e3ea94) at xlogutils.c:391
> #6  0x0055df59 in heap_xlog_prune (record=0x1183b98) at heapam.c:8779
> #7  heap2_redo (record=0x1183b98) at heapam.c:10015
> #8  0x005ca430 in ApplyWalRecord (replayTLI=,
> record=0x7f8f7afbcb60, xlogreader=)
> at ../../../../src/include/access/xlog_internal.h:379
>
> It's not clear to me whether this Assert is wrong, or
> XLogReadBufferForRedoExtended shouldn't be using
> RBM_ZERO_AND_CLEANUP_LOCK, or the Assert is correctly protecting an
> unimplemented case in ExtendBufferedRelTo that we now need to implement.

Hm. It's not implemented because I didn't quite see how it'd make sense to
pass RBM_ZERO_AND_CLEANUP_LOCK when extending the relation, but given how
relation extension is done "implicitly" during recovery, that's too narrow a
view. It's trivial to add.

I wonder if we should eventually redefine the RBM* things into a bitmask.


> In any case, I'm pretty sure Andres broke it in 26158b852, because
> I hadn't seen it before this weekend.

Yea, that's clearly the fault of 26158b852.

Greetings,

Andres Freund

Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized

[Tom Lane]

Justin Pryzby  writes:
> postgres=# SET force_parallel_mode =1; CREATE TABLE x (i int) PARTITION BY 
> RANGE (i); CREATE TABLE x1 PARTITION OF x DEFAULT ;
>  select * from pg_class,
>   lateral (select pg_catalog.bit_and(1)
>   from pg_class as sample_1
>   where case when EXISTS (
> select 1 from x
>   where EXISTS (
> select 1 from pg_catalog.pg_depend
>   where (sample_1.reltuples is NULL)
>   )) then 1 end
>is NULL)x
> where false;

Interesting.  The proximate cause is that we end up with a subplan
that is marked parallel_safe, but it has an initplan that is not
parallel_safe.  The parallel worker receives, and tries to initialize,
the parallel_safe subplan, and falls over because of its reference
to the unsafe subplan -- which was not transmitted to the worker.

Actually, because of the policy installed by commit ab77a5a45, the
mere fact of having an initplan should be enough to disqualify
the first subplan from being marked parallel-safe.

I dug around and found the culprit: setrefs.c's
clean_up_removed_plan_level() moves initplans down from a parent
to a child plan node, but it forgot the possibility that the
child plan node had been marked parallel_safe before that and
must not be anymore.

The v1 patch attached is enough to fix the immediate issue,
but there's another thing not to like, which is that we're also
discarding the costs associated with the initplans.  That's
strictly cosmetic given that all the planning decisions are
already made, but it still seems potentially annoying if you're
trying to understand EXPLAIN output.  So I'm inclined to instead
do something like v2 attached, which deals with that as well.
(On the other hand, we aren't bothering to fix up costs when
we move initplans around in materialize_finished_plan or
standard_planner ... so maybe that should be left for a patch
that fixes those things too.)

Another thing worth wondering about is whether we can't loosen
commit ab77a5a45's policy that having an initplan is enough
to make you parallel-unsafe.  In the wake of later fixes,
notably 5e6d8d2bb, it seems like maybe we could allow that
as long as the initplans themselves are parallel-safe.  That
wouldn't be material for back-patching though, so I'll worry
about it later.

Not sure what if anything to do about a test case.  I'm not
excited about memorializing the specific case found by sqlsmith,
because it seems only very accidental that it exposes this
problem.  I found that there are existing regression tests
that exercise the situation where clean_up_removed_plan_level
generates an incorrectly-marked plan, but there is accidentally
no bad effect.  (The planner itself isn't going to be making
any further decisions with the bogus info; it's only
ExecSerializePlan that pays attention to the flag, and we'd only
notice in this specific cross-reference situation.)  Also, any
change we make along the lines speculated about in the previous
para would be highly likely to break a test case, in the sense
that it'd no longer exercise the previously-failing scenario.
So on the whole I'm inclined not to bother with a new test case.

regards, tom lane

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b5398e..042036b11b 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1546,6 +1546,10 @@ clean_up_removed_plan_level(Plan *parent, Plan *child)
 	child->initPlan = list_concat(parent->initPlan,
   child->initPlan);
 
+	/* If we moved down any initplans, child is no longer parallel-safe */
+	if (child->initPlan)
+		child->parallel_safe = false;
+
 	/*
 	 * We also have to transfer the parent's column labeling info into the
 	 * child, else columns sent to client will be improperly labeled if this
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b5398e..9a7c8ea07b 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1542,7 +1542,31 @@ trivial_subqueryscan(SubqueryScan *plan)
 static Plan *
 clean_up_removed_plan_level(Plan *parent, Plan *child)
 {
-	/* We have to be sure we don't lose any initplans */
+	ListCell   *lc;
+
+	/*
+	 * We have to be sure we don't lose any initplans, so move any that were
+	 * attached to the parent plan to the child.  If we do move any, the child
+	 * is no longer parallel-safe.  As a cosmetic matter, also add the
+	 * initplans' run costs to the child's costs (compare
+	 * SS_charge_for_initplans).
+	 */
+	foreach(lc, parent->initPlan)
+	{
+		SubPlan*initsubplan = (SubPlan *) lfirst(lc);
+		Cost		initplan_cost;
+
+		child->parallel_safe = false;
+		initplan_cost = initsubplan->startup_cost + initsubplan->per_call_cost;
+		child->startup_cost += initplan_cost;
+		child->total_cost += initplan_cost;
+	}
+
+	/*
+	 * Attach plans this way so that parent's 

Re: [PATCH] Use role name "system_user" instead of "user" for unsafe_tests

[Tom Lane]

Aleksander Alekseev  writes:
>> I don't think we can protect against all possible user names. Wouldn't it be 
>> better to run the tests under an OS user with a different name, like 
>> "marmaduke"? ("user" is a truly terrible default user name).

> 100% agree. The point is not to protect against all possible user
> names but merely to reduce the likelihood of the problem.

It only reduces the likelihood if you assume that "system_user"
is less likely than "user" as a choice of OS user name to run
the tests under.  That seems like a debatable assumption;
perhaps it's actually *more* likely.

Whether we need to have a test for this at all is perhaps a
more interesting argument.

regards, tom lane

Re: Minimal logical decoding on standbys

[Andres Freund]

Hi,

On 2023-04-11 11:04:50 +0200, Drouvot, Bertrand wrote:
> On 4/11/23 10:55 AM, Drouvot, Bertrand wrote:
> > Hi,
> > 
> > I think we might want to add:
> > 
> > $node_primary->wait_for_replay_catchup($node_standby);
> > 
> > before calling the slot creation.
> > 
> > It's done in the attached, would it be possible to give it a try please?
> 
> Actually, let's also wait for the cascading standby to catchup too, like done 
> in the attached.

Pushed. Seems like a clear race in the test, so I didn't think it was worth
waiting for testing it on hoverfly.

I think we should lower the log level, but perhaps wait for a few more cycles
in case there are random failures?

I wonder if we should make the connections in poll_query_until to reduce
verbosity - it's pretty annoying how much that can bloat the log. Perhaps also
introduce some backoff? It's really annoying to have to trawl through all
those logs when there's a problem.

Greetings,

Andres Freund

infobits_set WAL record struct field is int8

[Peter Geoghegan]

Commit 0ac5ad5134 ("Improve concurrency of foreign key locking") added
infobits_set fields to certain WAL records. However, in the case of
xl_heap_lock, it made the data type int8 rather than uint8.

I believe that this was a minor oversight. Attached patch fixes the issue.

-- 
Peter Geoghegan


v1-0001-Fix-xl_heap_lock.infobits_set-type.patch
Description: Binary data

Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Andres Freund]

Hi,

On 2023-04-11 14:46:23 -0500, Justin Pryzby wrote:
> Yes, $SUBJECT is correct.
>
> On an old centos6 VM which I'd forgotten about and never removed from
> monitoring, I noticed that a process had recently crashed...
>
> Maybe this is an issue which was already fixed, but I looked and find no
> bug report nor patch about it.  Feel free to dismiss the problem report
> if it's not interesting or useful.

> postgres was compiled locally at 4e54d231a.  It'd been running
> continuously since September without crashing until a couple weeks ago
> (and running nearly-continuously for months before that).

It possibly could be:

Author: Andres Freund 
Branch: master [cb2e7ddfe] 2022-12-02 18:10:30 -0800
Branch: REL_15_STABLE Release: REL_15_2 [c6a60471a] 2022-12-02 18:07:47 -0800
Branch: REL_14_STABLE Release: REL_14_7 [6344bc097] 2022-12-02 18:10:30 -0800
Branch: REL_13_STABLE Release: REL_13_10 [7944d2d8c] 2022-12-02 18:13:40 -0800
Branch: REL_12_STABLE Release: REL_12_14 [35b99a18f] 2022-12-02 18:16:14 -0800
Branch: REL_11_STABLE Release: REL_11_19 [af3517c15] 2022-12-02 18:17:54 -0800
 
Prevent pgstats from getting confused when relkind of a relation changes

But the fact that it's on a catalog table's stats makes it less likely,
although not impossible.


Any chance there were conversions from tables to views in that connection?

Greetings,

Andres Freund

RE: [PATCH] Support % wildcard in extension upgrade filenames

[Regina Obe]

> Packager might actually know better in that they could ONLY consider the
> packages ever packaged by them.
> 
I'm a special case packager cause I'm on the PostGIS project and I only
package postgis related extensions, but even I find this painful.  
But for most packagers, I think they are juggling too many packages and too
many OS versions to micro manage the business of each package.
In my case my job is simple.  I deal just with Windows and that doesn't
change from Windows version to Windows version (just PG specific).
Think of upgrading from Debian 10 to Debian 12 - what would you as a PG
packager expect people to be running and upgrading from?
They could be switching from say the main distro to the pgdg distro.

> Hey, best would be having support for wildcard wouldn't it ?
> 
For PostGIS yes and any other extension that does nothing but add new
functions or replaces existing ones.   For others some minor handling would
be ideal, though I guess some other projects would be happy with a wildcard
(e.g. pgRouting  would prefer a wildcard) since most of the changes are just
additions of new functions or replacements of existing functions.

For something like h3-pg I think a simpler micro handling would be ideal,
though not sure.
They ship two extensions (one that is a bridge to postgis and their newest
takes advantage of postgis_raster too)
https://github.com/zachasme/h3-pg/tree/main/h3_postgis/sql/updates

https://github.com/zachasme/h3-pg/tree/main/h3/sql/updates 
Many of their upgrades are No-ops cause they really are just lib upgrades.


I'm thinking maybe we should discuss these ideas with projects who would
benefit most from this:

(many of which I'm familiar with because they are postgis offsprings and I
package or plan to package them  - pgRouting, h3-pg, pgPointCloud,
mobilityDb, 

Not PostGIS offspring:

ZomboDB - https://github.com/zombodb/zombodb/tree/master/sql  -  (most of
those are just changing versioning on the function call, so yah wildcard
would be cleaner there)
TimescaleDB - https://github.com/timescale/timescaledb/tree/main/sql/updates
( I don't think a wildcard would work here especially since they have some
downgrade paths, but is a useful example of a micro-level extension upgrade
pattern we should think about if we could make easier)
https://github.com/timescale/timescaledb/tree/main/sql/updates  


> > I much preferred the idea of just listing all our upgrade targets in the
> control file.
> 
> Again: how would we know all upgrade targets ?
> 
We already do, remember you wrote it  :)

https://git.osgeo.org/gitea/postgis/postgis/src/branch/master/extensions/upg
radeable_versions.mk

Yes it does require manual updating each release cycle (and putting in
versions from just released stable branches).  I can live with continuing
with that exercise.  It was a nice to have but is not nearly as annoying as
1000 scripts or as a packager trying to catalog what versions of packages
have I released that I need to worry about.


> > We need to come up with a convention of how to describe a micro
> > update, as it's really a problem with extensions that follow the
> > pattern
> 
> I think it's a problem with extensions maintaining stable branches, as if
the
> history was linear we would possibly need less files (although at this
stage
> any number bigger than 1 would be too much for me)
> 
> --strk;

I'm a woman of compromise. Sure 1 file would be ideal, but 
I'd rather live with a big file listing all version upgrades than 1000 files
with the same information.

It's cleaner to read a single file than make sense of a pile of files.

Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Andres Freund]

Hi,

On 2023-04-11 13:35:38 -0700, Andres Freund wrote:
> On 2023-04-11 14:46:23 -0500, Justin Pryzby wrote:
> > Yes, $SUBJECT is correct.
> >
> > On an old centos6 VM which I'd forgotten about and never removed from
> > monitoring, I noticed that a process had recently crashed...
> >
> > Maybe this is an issue which was already fixed, but I looked and find no
> > bug report nor patch about it.  Feel free to dismiss the problem report
> > if it's not interesting or useful.
> 
> > postgres was compiled locally at 4e54d231a.  It'd been running
> > continuously since September without crashing until a couple weeks ago
> > (and running nearly-continuously for months before that).
> 
> It possibly could be:
> 
> Author: Andres Freund 
> Branch: master [cb2e7ddfe] 2022-12-02 18:10:30 -0800
> Branch: REL_15_STABLE Release: REL_15_2 [c6a60471a] 2022-12-02 18:07:47 -0800
> Branch: REL_14_STABLE Release: REL_14_7 [6344bc097] 2022-12-02 18:10:30 -0800
> Branch: REL_13_STABLE Release: REL_13_10 [7944d2d8c] 2022-12-02 18:13:40 -0800
> Branch: REL_12_STABLE Release: REL_12_14 [35b99a18f] 2022-12-02 18:16:14 -0800
> Branch: REL_11_STABLE Release: REL_11_19 [af3517c15] 2022-12-02 18:17:54 -0800
>  
> Prevent pgstats from getting confused when relkind of a relation changes
> 
> But the fact that it's on a catalog table's stats makes it less likely,
> although not impossible.
> 
> 
> Any chance there were conversions from tables to views in that connection?

Nope, not possible - the stack trace actually shows this is during connection 
establishment.

Thomas, see stack trace upthread?

Greetings,

Andres Freund

Re: Minimal logical decoding on standbys

[Tom Lane]

Andres Freund  writes:
> I think we should lower the log level, but perhaps wait for a few more cycles
> in case there are random failures?

Removing

-log_min_messages = 'debug2'
-log_error_verbosity = verbose

not only reduces 035's log output volume from 1.6MB to 260kB,
but also speeds it up nontrivially: on my machine it takes
about 8.50 sec as of HEAD, but 8.09 sec after silencing the
extra logging.  So I definitely want to see that out of there.

regards, tom lane

Re: is_superuser is not documented

[Joseph Koshakow]

On Tue, Apr 11, 2023 at 9:37 AM Fujii Masao 
wrote:

>>  > Yes, this patch moves the descriptions of is_superuser to
config.sgml
>>  > and changes its group to PRESET_OPTIONS.
>>
>> is_superuser feels a little out of place in this file. All of
>> the options here apply to the entire PostgreSQL server, while
>> is_superuser only applies to the current session.
>
>Aren't other preset options like lc_collate, lc_ctype and
server_encoding
>similar to is_superuser? They seem to behave in a similar way as their
>settings can be different for each connection depending on the
connected database.

I think the difference is that all of those options are constant for
all connections to the same database and once the database is created
they are immutable. is_superuser is set on a per session basis and can
be changed at any time.

Looking through the options it actually looks like all the options are
set either when the server is built, the server is started, or the
database is created, and once they're set they become immutable. The
one exception I see is in_hot_standby mode which can be updated from on
to off (I can't remember off the top of my head if it can be updated
the other way). I'm moving the goal post a bit but I think preset may
imply that the value isn't going to change once it's been set.

Having said all that I actually think this is the best place for
is_superuser since it doesn't seem to fit in anywhere else.

>> I'm not familiar with the origins of is_superuser and it may be too
>> late for this, but it seems like is_superuser would fit in much
better
>> as a system information function [0] rather than a GUC. Particularly
>> in the Session Information Functions.
>
>I understand your point, but I think it would be more confusing to
document
>is_superuser there because it's defined and behaves differently from
>session information functions like current_user. For instance,
>the value of is_superuser can be displayed using SHOW command,
>while current_user cannot. Therefore, it's better to keep is_superuser
>separate from the session information functions.

I was implying that I thought it would have made more sense for
is_superuser to be implemented as a function, behave as a function,
and not be visible via SHOW. However, there may have been a good reason
not to do this and it may already be too late for that.

In my opinion, this is ready to be committed. However, like I said
earlier I'm not very familiar with the GUC code so you may want to
wait for another opinion.

Thanks,
Joe Koshakow

Re: infobits_set WAL record struct field is int8

[Andres Freund]

Hi,

On 2023-04-11 13:13:49 -0700, Peter Geoghegan wrote:
> Commit 0ac5ad5134 ("Improve concurrency of foreign key locking") added
> infobits_set fields to certain WAL records. However, in the case of
> xl_heap_lock, it made the data type int8 rather than uint8.
>
> I believe that this was a minor oversight. Attached patch fixes the issue.

Makes sense. Looks like there never was a flag defined for the sign bit,
luckily. I assume you're just going to apply this for HEAD?

Greetings,

Andres Freund

Re: Assertion being hit during WAL replay

[Tom Lane]

Andres Freund  writes:
> On 2023-04-11 14:48:44 -0400, Tom Lane wrote:
>> I have seen this failure a couple of times recently while
>> testing code that caused crashes and restarts:

> Do you have a quick repro recipe?

Here's something related to what I hit that time:

diff --git a/src/backend/optimizer/plan/subselect.c 
b/src/backend/optimizer/plan/subselect.c
index 052263aea6..d43a7c7bcb 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2188,6 +2188,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo 
*final_rel)
 void
 SS_attach_initplans(PlannerInfo *root, Plan *plan)
 {
+   Assert(root->init_plans == NIL);
plan->initPlan = root->init_plans;
 }
 
You won't get through initdb with this, but if you install this change
into a successfully init'd database and then "make installcheck-parallel",
it will crash and then fail to recover, at least a lot of the time.

regards, tom lane

Re: infobits_set WAL record struct field is int8

[Peter Geoghegan]

On Tue, Apr 11, 2023 at 1:48 PM Andres Freund  wrote:
> Makes sense. Looks like there never was a flag defined for the sign bit,
> luckily. I assume you're just going to apply this for HEAD?

Yes.

I'm also going to rename the TransactionId field to "xmax", for
consistency with nearby very similar records (like
xl_heap_lock_updated).


-- 
Peter Geoghegan

Re: Various typo fixes

[Daniel Gustafsson]

> On 11 Apr 2023, at 16:53, Justin Pryzby  wrote:

> I think "logical" should be a  here.

Agree, it should in order to be consistent.

--
Daniel Gustafsson

Re: When to drop src/tools/msvc support

[Alvaro Herrera]

On 2023-Apr-11, Michael Paquier wrote:

> Getting a CI job able to do some validation for MSVC would be indeed
> nice.  What's the plan in the buildfarm with this coverage?  Would all
> the animals switch to meson (Chocolatey + StrawberryPerl, I assume)
> for the job or will there still be some coverage with MSVC for v16
> there?

If we keep MSVC support in 16, then I agree we should have a CI task for
it -- and IMO we should make ti automatically triggers whenever any
Makefile or meson.build is modified.  Hopefully we won't touch them
much now that the branch is feature-frozen, but it could still happen.

Do we have code for it already, even if incomplete?

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

Re: [PATCH] Support % wildcard in extension upgrade filenames

[Sandro Santilli]

On Tue, Apr 11, 2023 at 04:36:04PM -0400, Regina Obe wrote:
> 
> > Hey, best would be having support for wildcard wouldn't it ?
> 
> I'm a woman of compromise. Sure 1 file would be ideal, but 
> I'd rather live with a big file listing all version upgrades
> than 1000 files with the same information.

Personally I don't see the benefit of 1 big file vs. many 0-length
files to justify the cost (time and complexity) of a PostgreSQL change,
with the corresponding cost of making use of this new functionality
based on PostgreSQL version.

We'd still have the problem of missing upgrade paths unless we release
a new version of PostGIS even if it's ONLY for the sake of updating
that 1 big file (or adding a new file, in the current situation).

--strk;

Re: Show various offset arrays for heap WAL records

[Melanie Plageman]

On Tue, Apr 11, 2023 at 1:35 PM Peter Geoghegan  wrote:
>
> On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman  
> wrote:
> > Not the fault of this patch, but I also noticed that heap UPDATE and
> > HOT_UPDATE records have xmax twice and don't differentiate between new
> > and old. I think that was probably a mistake.
> >
> > description  | off: 119, xmax: 1105, flags: 0x00, old_infobits:
> > [], new off: 100, xmax 0
>
> That doesn't seem great to me either. I don't like this ambiguity,
> because it seems like it makes the description hard to parse in a way
> that flies in the face of what we're trying to do here, in general.
> So it seems like it might be worth fixing now, in the scope of this
> patch.

Agreed.

On Tue, Apr 11, 2023 at 3:22 PM Peter Geoghegan  wrote:
>
> On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan  wrote:
> > Attached revision deals with this by spelling out the names in full
> > (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields
> > to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL
> > record types, on the theory that those should match the physical
> > record (unless there is a good reason not to, which doesn't apply
> > here).
>
> I just noticed that we don't even show xmax in the case of DELETE
> records. Perhaps the original assumption is that it must match the
> record's own XID, but that's not true after the MultiXact enhancements
> for foreign key locking added to 9.3 (and in any case there is no
> reason at all to show xmax in UPDATE but not in DELETE).
>
> Attached revision v4 fixes this, making DELETE, UPDATE, HOT_UPDATE,
> LOCK, and LOCK_UPDATED record types consistent with each other in
> terms of the key names output by the heap desc routine. The field
> order also needed a couple of tweaks for struct consistency (and
> cross-record consistency) for v4.

Code in v4 all seems fine to me.
I like the update guidelines comment.

I agree it would be nice for xl_heap_lock->locking_xid to be renamed
xmax for clarity. I would suggest that if you don't intend to put it
in a separate commit, you mention it explicitly in the final commit
message. Its motivation isn't immediately obvious to the reader.

- Melanie

Re: Documentation for building with meson

[samay sharma]

Hi,

On Tue, Apr 11, 2023 at 10:18 AM Andres Freund  wrote:

> Hi,
>
> On 2023-03-28 12:27:26 -0700, samay sharma wrote:
> > Subject: [PATCH v9 1/5] Make minor additions and corrections to meson
> docs
> >
> > This commit makes a few corrections to the meson docs
> > and adds a few instructions and links for better clarity.
> > ---
> >  doc/src/sgml/installation.sgml | 24 +++-
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/doc/src/sgml/installation.sgml
> b/doc/src/sgml/installation.sgml
> > index 70ab5b77d8..e3b9b6c0d0 100644
> > --- a/doc/src/sgml/installation.sgml
> > +++ b/doc/src/sgml/installation.sgml
> > @@ -2057,8 +2057,7 @@ meson setup build -Dssl=openssl
> >  
> >  meson configure -Dcassert=true
> >  
> > -meson configure's commonly used command-line
> options
> > -are explained in .
> > +Commonly used build options for meson configure
> (and meson setup) are explained in  linkend="meson-options"/>.
> > 
> >
> >
> > @@ -2078,6 +2077,13 @@ ninja
> >  processes used with the command line argument -j.
> > 
> >
> > +   
> > +If you want to build the docs, you can type:
> > +
> > +ninja docs
> > +
> > +   
>
> "type" sounds a bit too, IDK, process oriented. "To build the docs, use"?
>

Sure, that works.

>
>
> > Subject: [PATCH v9 2/5] Add data layout options sub-section in
> installation
> >  docs
> >
> > This commit separates out blocksize, segsize and wal_blocksize
> > options into a separate Data layout options sub-section in both
> > the make and meson docs. They were earlier in a miscellaneous
> > section which included several unrelated options. This change
> > also helps reduce the repetition of the warnings that changing
> > these parameters breaks on-disk compatibility.
>
> Makes sense. I'm planning to apply this unless Peter or somebody else has
> further feedback.
>

Cool.

>
>
> > From 11d82aa49efb3d1cbc08f14562a757f115053c8b Mon Sep 17 00:00:00 2001
> > From: Samay Sharma 
> > Date: Mon, 13 Feb 2023 16:23:52 -0800
> > Subject: [PATCH v9 3/5] Remove Anti-Features section from Installation
> from
> >  source docs
> >
> > Currently, several meson setup options are listed in anti-features.
> > However, they are similar to most other options in the postgres
> > features list as they are 'auto' features themselves. Also, other
> > options are likely better suited to the developer options section.
> > This commit, therefore, moves the options listed in the anti-features
> > section into other sections and removes that section.
> >
> > For consistency, this reorganization has been done on the make section
> > of the docs as well.
>
> Makes sense to me. "Anti-Features" is confusing as a name to start with.
>
> Greetings,
>
> Andres Freund
>

Re: Can we do something to help stop users mistakenly using force_parallel_mode?

[David Rowley]

On Thu, 16 Feb 2023 at 00:05, David Rowley  wrote:
> I pushed the rename patch earlier.
>
> How should we go about making contact with the owners?

After a quick round of making direct contact with the few remaining
buildfarm machine owners which are still using force_parallel_mode,
we're now down to just one remainder (warbler).

In preparation for when that's ticked off, I'd like to gather people's
thoughts about if we should remove force_parallel_mode from v16?

My thoughts are that providing we can get the remaining animal off it
and remove the GUC alias before beta1, we should remove it.  Renaming
the GUC to debug_parallel_query was entirely aimed at breaking things
for people who are (mistakenly) using it, so I don't see why we
shouldn't remove it.

Does anyone feel differently?

David

Re: Can we do something to help stop users mistakenly using force_parallel_mode?

[Tom Lane]

David Rowley  writes:
> In preparation for when that's ticked off, I'd like to gather people's
> thoughts about if we should remove force_parallel_mode from v16?

To clarify, you just mean removing that alias, right?  +1.
I don't see a reason to wait longer once the buildfarm is on board.

regards, tom lane

Re: ERROR messages in VACUUM's PARALLEL option

[David Rowley]

(On Wed, 12 Apr 2023 at 01:58, Tom Lane  wrote:
>
> David Rowley  writes:
> > Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> > BUFFER_USAGE_LIMIT option.
>
> > 1) buffer_usage_limit in the ERROR messages should be consistently in 
> > uppercase.
>
> FWIW, I think this is exactly backward, and so is whatever code you
> based this on.  Our usual habit is to write GUC names and suchlike
> in lower case in error messages.  Add double quotes if you feel you
> want to set them off from the surrounding text.  Here's a typical
> example of longstanding style:

Thanks for chipping in on this. Can you confirm if you think this
should apply to VACUUM options?  We're not talking GUCs here.

I think there might be some precedents creeping in here for the legacy
VACUUM syntax.  Roughly the current ERROR messages are using upper
case. e.g:

"PROCESS_TOAST required with VACUUM FULL"
"VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL"
"ANALYZE option must be specified when a column list is provided"

It's quite possible the newer options copied what VACUUM FULL did, and
VACUUM FULL was probably upper case from before we had the newer
syntax with the options in parenthesis.

I did notice that defGetString() did lower case, but that not exactly
terrible. It seems worse to have ExecVacuum() use a random assortment
of casings when reporting errors in the specified options. Unless
someone thinks are should edit all of those to lower case, then I
think the best option is just to make BUFFER_USAGE_LIMIT follow the
existing precedent of upper casing.

David

Re: Can we do something to help stop users mistakenly using force_parallel_mode?

[David Rowley]

On Wed, 12 Apr 2023 at 09:53, Tom Lane  wrote:
>
> David Rowley  writes:
> > In preparation for when that's ticked off, I'd like to gather people's
> > thoughts about if we should remove force_parallel_mode from v16?
>
> To clarify, you just mean removing that alias, right?  +1.
> I don't see a reason to wait longer once the buildfarm is on board.

Yip, alias. i.e:

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea67cfa5e5..7d3b20168a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -186,7 +186,6 @@ static const unit_conversion time_unit_conversion_table[] =
 static const char *const map_old_guc_names[] = {
"sort_mem", "work_mem",
"vacuum_mem", "maintenance_work_mem",
-   "force_parallel_mode", "debug_parallel_query",
NULL
 };

David

Re: Assertion being hit during WAL replay

[Andres Freund]

Hi,

On 2023-04-11 16:54:53 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2023-04-11 14:48:44 -0400, Tom Lane wrote:
> >> I have seen this failure a couple of times recently while
> >> testing code that caused crashes and restarts:
> 
> > Do you have a quick repro recipe?
> 
> Here's something related to what I hit that time:
> 
> diff --git a/src/backend/optimizer/plan/subselect.c 
> b/src/backend/optimizer/plan/subselect.c
> index 052263aea6..d43a7c7bcb 100644
> --- a/src/backend/optimizer/plan/subselect.c
> +++ b/src/backend/optimizer/plan/subselect.c
> @@ -2188,6 +2188,7 @@ SS_charge_for_initplans(PlannerInfo *root, RelOptInfo 
> *final_rel)
>  void
>  SS_attach_initplans(PlannerInfo *root, Plan *plan)
>  {
> +   Assert(root->init_plans == NIL);
> plan->initPlan = root->init_plans;
>  }
>  
> You won't get through initdb with this, but if you install this change
> into a successfully init'd database and then "make installcheck-parallel",
> it will crash and then fail to recover, at least a lot of the time.

Ah, that allowed me to reproduce. Thanks.


Took me a bit to understand how we actually get into this situation. A PRUNE
record for relation+block that doesn't exist during recovery. That doesn't
commonly happen outside of PITR or such, because we obviously need a block
with content to generate the PRUNE. The way it does happen here, is that the
relation is vacuumed and then truncated. Then we crash. Thus we end up with a
PRUNE record for a block that doesn't exist on disk.

Which is also why the test is quite timing sensitive.

Seems like it'd be good to have a test that covers this scenario. There's
plenty code around it that doesn't currently get exercised.

None of the existing tests seem like a great fit. I guess it could be added to
013_crash_restart, but that really focuses on something else.

So I guess I'll write a 036_notsureyet.pl...

Greetings,

Andres Freund

Re: Various typo fixes

[Justin Pryzby]

On Tue, Apr 11, 2023 at 09:53:00AM -0500, Justin Pryzby wrote:
> On Tue, Apr 11, 2023 at 03:43:12PM +0100, Thom Brown wrote:
> > On Tue, 11 Apr 2023 at 15:39, Justin Pryzby  wrote:
> > >
> > > On Tue, Apr 11, 2023 at 03:36:02PM +0100, Thom Brown wrote:
> > > > I've attached a patch with a few typo and grammatical fixes.
> > >
> > > I think you actually sent the "git-diff" manpage :(
> > 
> > Oh dear, well that's a first.  Thanks for pointing out.
> 
> Thanks.  I think these are all new in v16, right ?
> 
> I noticed some of these too - I'll send a patch pretty soon.

The first attachment fixes for typos in user-facing docs new in v16,
combining Thom's changes with the ones that I'd found.  If that's
confusing, I'll resend my patches separately.

The other four numbered patches could use extra review.

-- 
Justin
diff --git a/doc/src/sgml/archive-modules.sgml 
b/doc/src/sgml/archive-modules.sgml
index 7cf44e82e23..7064307d9e6 100644
--- a/doc/src/sgml/archive-modules.sgml
+++ b/doc/src/sgml/archive-modules.sgml
@@ -50,7 +50,7 @@
_PG_archive_module_init.  The result of the function
must be a pointer to a struct of type
ArchiveModuleCallbacks, which contains everything
-   that the core code needs to know how to make use of the archive module.  The
+   that the core code needs to know to make use of the archive module.  The
return value needs to be of server lifetime, which is typically achieved by
defining it as a static const variable in global scope.
 
@@ -82,7 +82,7 @@ typedef const ArchiveModuleCallbacks *(*ArchiveModuleInit) 
(void);

 The startup_cb callback is called shortly after the
 module is loaded.  This callback can be used to perform any additional
-initialization required.  If the archive module has a state, it can use
+initialization required.  If the archive module has any state, it can use
 state->private_data to store it.
 
 
@@ -143,7 +143,7 @@ typedef bool (*ArchiveFileCB) (ArchiveModuleState *state, 
const char *file, cons
 process exits (e.g., after an error) or the value of
  changes.  If no
 shutdown_cb is defined, no special action is taken in
-these situations.  If the archive module has a state, this callback should
+these situations.  If the archive module has any state, this callback 
should
 free it to avoid leaks.
 
 
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b9d73deced2..bcdc085010a 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -810,7 +810,7 @@ hostall all ::1/128 
trust
 hostall all localhost   trust
 
 # The same using a regular expression for DATABASE, that allows connection
-# to the database db1, db2 and any databases with a name beginning by "db"
+# to the database db1, db2 and any databases with a name beginning with "db"
 # and finishing with a number using two to four digits (like "db1234" or
 # "db12").
 #
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f81c2045ec4..20b8b5ae1de 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11835,7 +11835,7 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) 
id(24688,24696,0,0,0,1)
 option of
 CREATE 
SUBSCRIPTION
 is enabled, otherwise, serialize each change.  When set to
-buffered, the decoding will stream or serialize
+buffered, decoding will stream or serialize
 changes when logical_decoding_work_mem is reached.

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e6a7514100e..9c3b2018896 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27084,8 +27084,9 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * 
ps.setting::int + :offset


 Take a snapshot of running transactions and write it to WAL, without
-having to wait bgwriter or checkpointer to log one. This is useful for
-logical decoding on standby, as logical slot creation has to wait
+having to wait for bgwriter or
+checkpointer to log one. This is useful for
+logical decoding on a standby, as logical slot creation has to wait
 until such a record is replayed on the standby.

   
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 29ece7c42ee..e813e2b620a 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -257,7 +257,7 @@ typedef struct IndexAmRoutine
Access methods that do not point to individual tuples, but to block ranges
(like BRIN), may allow the HOT 
optimization
to continue. This does not apply to attributes referenced in index
-   predicates, an update of such attribute always disables 
HOT.
+   predicates, an update of such an attribute always disables 
HOT.
   
 
  
diff --git a/doc/src/sgml/install-windows.sgml 
b/doc/src/sgml/install-windows.sgml
index 2db44db2

Re: ERROR messages in VACUUM's PARALLEL option

[Tom Lane]

David Rowley  writes:
> Thanks for chipping in on this. Can you confirm if you think this
> should apply to VACUUM options?  We're not talking GUCs here.

My druthers would be to treat them similarly to GUCs.
I recognize that I might be in the minority, and that doing
so would entail touching a lot of existing messages in this
area.  Nonetheless, I think this area is not being consistent
with our wider conventions, which is why for example defGetString
is out of step with these messages.

regards, tom lane

RE: [PATCH] Support % wildcard in extension upgrade filenames

[Regina Obe]

> Personally I don't see the benefit of 1 big file vs. many 0-length files
to justify
> the cost (time and complexity) of a PostgreSQL change, with the
> corresponding cost of making use of this new functionality based on
> PostgreSQL version.
> 
>From a  packaging stand-point 1 big file is better than tons of 0-length
files.  
Fewer files to uninstall and to double check when testing.

As to the added complexity agree it's more but in my mind worth it if we
could get rid of this mountain of files.
But my vote would be the wild-card solution as I think it would serve more
than just postgis need.
Any project that rarely does anything but  add, remove, or modify functions
doesn't really need multiple upgrade scripts and 
I think quite a few extensions fall in that boat.

> We'd still have the problem of missing upgrade paths unless we release a
new
> version of PostGIS even if it's ONLY for the sake of updating that 1 big
file (or
> adding a new file, in the current situation).
> 
> --strk;

I think we always have more releases with newer stable versions than older
stable versions.
I can't remember a case when we had this ONLY issue.
If there is one fix in an older stable, version we usually wait for several
more before bothering with a release 
and then all the stables are released around the same time.  So I don't see
the ONLY being a real problem.

Re: When to drop src/tools/msvc support

[Andres Freund]

Hi,

On 2023-04-11 19:44:20 +0200, Alvaro Herrera wrote:
> On 2023-Apr-11, Michael Paquier wrote:
>
> > Getting a CI job able to do some validation for MSVC would be indeed
> > nice.  What's the plan in the buildfarm with this coverage?  Would all
> > the animals switch to meson (Chocolatey + StrawberryPerl, I assume)
> > for the job or will there still be some coverage with MSVC for v16
> > there?
>
> If we keep MSVC support in 16, then I agree we should have a CI task for
> it -- and IMO we should make ti automatically triggers whenever any
> Makefile or meson.build is modified.  Hopefully we won't touch them
> much now that the branch is feature-frozen, but it could still happen.

Once 16 branched, we can just have it always run, I think. It's just the
development branch where it's worth avoiding that (for cfbot and personal
hackery).

I guess we could do something like:

  manual: "changesInclude('**.meson.build', '**Makefile*', '**.mk', 
'src/tools/msvc/**')"

so the task would be manual triggered if none of those files change. If you
have write rights on the repository in question, you can trigger manual tasks
with a click.


> Do we have code for it already, even if incomplete?

My meson branch has a commit adding a bunch of additional tasks. Including
building with src/tools/msvc, building with meson + msbuild, openbsd, netbsd.

https://github.com/postgres/postgres/commit/8f7c2ffb5a5e8f0ef3722e2439484187c1356416

Currently src/tools/msvc does build successfully, although the tests haven't
finished yet:
https://cirrus-ci.com/build/6298699714789376


(the cause for the opensuse failure is known, need to find cycles to tackle
that, not related to meson)

Greetings,

Andres Freund

Re: Show various offset arrays for heap WAL records

[Peter Geoghegan]

On Tue, Apr 11, 2023 at 2:29 PM Melanie Plageman
 wrote:
> > That doesn't seem great to me either. I don't like this ambiguity,
> > because it seems like it makes the description hard to parse in a way
> > that flies in the face of what we're trying to do here, in general.
> > So it seems like it might be worth fixing now, in the scope of this
> > patch.
>
> Agreed.

Great -- pushed a fix for this just now, which included that change.

> I agree it would be nice for xl_heap_lock->locking_xid to be renamed
> xmax for clarity. I would suggest that if you don't intend to put it
> in a separate commit, you mention it explicitly in the final commit
> message. Its motivation isn't immediately obvious to the reader.

What I ended up doing is making that part of a bug fix for a minor
buglet I noticed in passing -- it became part of the "Fix xl_heap_lock
WAL record field's data type" commit from a bit earlier on.

Thanks for your help with the follow-up work. Seems like we're done
with this now.

-- 
Peter Geoghegan

Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Thomas Munro]

On Wed, Apr 12, 2023 at 7:46 AM Justin Pryzby  wrote:
> Unfortunately:
> (gdb) p area->control->handle
> $3 = 0
> (gdb) p segment_map->header->magic
> value has been optimized out
> (gdb) p index
> $4 = 

Hmm, well index I can find from parameters:

> #2  0x00991470 in ExceptionalCondition (conditionName= optimized out>, errorType=, fileName= out>, lineNumber=1770) at assert.c:69
> #3  0x009b9f97 in get_segment_by_index (area=0x22818c0, index= optimized out>) at dsa.c:1769
> #4  0x009ba192 in dsa_get_address (area=0x22818c0, dp=1099511703168) 
> at dsa.c:953

We have dp=1099511703168 == 0x1012680, so index == 1 and the rest
is the offset into that segment.  It's not the initial segment in the
main shared memory area created by the postmaster with
dsa_create_in_place() (that'd be index 0), it's in an extra segment
that was created with shm_open().  We managed to open and mmap() that
segment, but it contains unexpected garbage.

Can you print *area->control?  And then can you see that the DSM
handle is in index 1 in "segment_handles" in there?  Then can you see
if your system has a file with that number in its name under
/dev/shm/, and can you tell me what "od -c /dev/shm/..." shows as the
first few lines of stuff at the top, so we can see what that
unexpected garbage looks like?

Side rant:  I don't think there's any particular indication that it's
the issue here, but while it's on my mind:  I really wish we didn't
use random numbers for DSM handles.  I understand where it came from:
the need to manage SysV shmem keyspace (a DSM mode that almost nobody
uses, but whose limitations apply to all modes).  We've debugged
issues relating to handle collisions before, causing unrelated DSM
segments to be confused, back when the random seed was not different
in each backend making collisions likely.  For every other mode, we
could instead use something like (slot, generation) to keep collisions
as far apart as possible (generation wraparound), and avoid collisions
between unrelated clusters by using the pgdata path as a shm_open()
prefix.  Another idea is to add a new DSM mode that would use memfd
and similar things and pass fds between backends, so that the segments
are entirely anonymous and don't need to be cleaned up after a crash
(I thought about that while studying the reasons why PostgreSQL can't
run on Capsicum (a capabilities research project) or Android (a
telephone), both of which banned SysV *and* POSIX shm because
system-global namespaces are bad).

Re: refactoring relation extension and BufferAlloc(), faster COPY

[Andres Freund]

Hi,

On 2023-04-11 22:00:00 +0300, Alexander Lakhin wrote:
> A few days later I've found a new defect introduced with 31966b151.

That's the same issue that Tom also just reported, at
https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us

Attached is my WIP fix, including a test.

Greetings,

Andres Freund
>From fbe84381fa39a48f906358ade0a64e93b81c05c9 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 11 Apr 2023 16:04:44 -0700
Subject: [PATCH v1] WIP: Fix ExtendBufferedRelTo() assert failure in recovery
 + tests

Reported-by: Alexander Lakhin 
Reported-by: Tom Lane 
Discussion: https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us
Discussion: https://postgr.es/m/0b5eb82b-cb99-e0a4-b932-3dc60e2e3...@gmail.com
---
 src/backend/storage/buffer/bufmgr.c  |  4 +-
 src/test/recovery/meson.build|  1 +
 src/test/recovery/t/036_truncated_dropped.pl | 99 
 3 files changed, 102 insertions(+), 2 deletions(-)
 create mode 100644 src/test/recovery/t/036_truncated_dropped.pl

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7778dde3e57..4f2c46a4339 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -889,7 +889,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
 	Assert(eb.smgr == NULL || eb.relpersistence != 0);
 	Assert(extend_to != InvalidBlockNumber && extend_to > 0);
 	Assert(mode == RBM_NORMAL || mode == RBM_ZERO_ON_ERROR ||
-		   mode == RBM_ZERO_AND_LOCK);
+		   mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK);
 
 	if (eb.smgr == NULL)
 	{
@@ -933,7 +933,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
 	 */
 	current_size = smgrnblocks(eb.smgr, fork);
 
-	if (mode == RBM_ZERO_AND_LOCK)
+	if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
 		flags |= EB_LOCK_TARGET;
 
 	while (current_size < extend_to)
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index e834ad5e0dc..20089580100 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -41,6 +41,7 @@ tests += {
   't/033_replay_tsp_drops.pl',
   't/034_create_database.pl',
   't/035_standby_logical_decoding.pl',
+  't/036_truncated_dropped.pl',
 ],
   },
 }
diff --git a/src/test/recovery/t/036_truncated_dropped.pl b/src/test/recovery/t/036_truncated_dropped.pl
new file mode 100644
index 000..571cff9639a
--- /dev/null
+++ b/src/test/recovery/t/036_truncated_dropped.pl
@@ -0,0 +1,99 @@
+# Copyright (c) 2021-2023, PostgreSQL Global Development Group
+
+# Tests recovery scenarios where the files are shorter than in the common
+# cases, e.g. due to replaying WAL records of a relation that was subsequently
+# truncated.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('n1');
+
+$node->init();
+
+# Disable autovacuum to guarantee VACUUM can remove rows / truncate relations
+$node->append_conf('postgresql.conf', qq[
+wal_level = 'replica'
+autovacuum = off
+]);
+
+$node->start();
+
+
+# Test replay of PRUNE records for blocks that are later truncated. With FPIs
+# used for PRUNE.
+
+$node->safe_psql(
+	'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = 1;
+CHECKPOINT; -- generate FPIs
+VACUUM truncme; -- generate prune records
+TRUNCATE truncme; -- make blocks non-existing
+INSERT INTO truncme SELECT generate_series(1, 10);
+]);
+
+$node->stop('immediate');
+
+ok($node->start(), 'replay of PRUNE records affecting truncated block (FPIs)');
+
+is($node->safe_psql('postgres', 'select count(*), sum(i) FROM truncme'),
+   '10|55',
+   'table contents as expected after recovery');
+$node->safe_psql('postgres', 'DROP TABLE truncme');
+
+
+# Test replay of PRUNE records for blocks that are later truncated. Without
+# FPIs used for PRUNE.
+
+$node->safe_psql(
+	'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = 1;
+VACUUM truncme; -- generate prune records
+TRUNCATE truncme; -- make blocks non-existing
+INSERT INTO truncme SELECT generate_series(1, 10);
+]);
+
+$node->stop('immediate');
+
+ok($node->start(), 'replay of PRUNE records affecting truncated block (no FPIs)');
+
+is($node->safe_psql('postgres', 'select count(*), sum(i) FROM truncme'),
+   '10|55',
+   'table contents as expected after recovery');
+$node->safe_psql('postgres', 'DROP TABLE truncme');
+
+
+# Test replay of partial relation truncation via VACUUM
+
+$node->safe_psql(
+	'postgres', qq[
+CREATE TABLE truncme(i int) WITH (fillfactor = 50);
+INSERT INTO truncme SELECT generate_series(1, 1000);
+UPDATE truncme SET i = i + 1;
+-- ensure a mix of pre/post truncation rows
+DELETE FROM truncme WHERE i > 500;
+
+VACUUM truncme; -- should truncate relation
+
+-- rows at TIDs that previo

Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Justin Pryzby]

On Wed, Apr 12, 2023 at 11:18:36AM +1200, Thomas Munro wrote:
> Can you print *area->control?

(gdb) p *area->control
$1 = {segment_header = {magic = 216163848, usable_pages = 62, size = 1048576, 
prev = 1, next = 18446744073709551615, bin = 4, freed = false}, handle = 0, 
segment_handles = {0, 3696856876, 433426374, 1403332952, 2754923922, 
0 }, segment_bins = {18446744073709551615, 
18446744073709551615, 18446744073709551615, 18446744073709551615, 3, 4, 
18446744073709551615, 18446744073709551615, 18446744073709551615, 
18446744073709551615, 18446744073709551615, 18446744073709551615, 
18446744073709551615, 18446744073709551615, 18446744073709551615, 
18446744073709551615}, pools = {{lock = {tranche = 72, state = {value = 
536870912}, 
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 
2199025295360, 8192, 0}}, {lock = {tranche = 72, state = {value = 536870912}, 
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 
2199025296088, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 
0, 0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = {
  head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock 
= {tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647, 
tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, tail = 
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 
0, 0, 0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters 
= {head = 2147483647, tail = 2147483647}}, spans = {0, 2199025296648, 
2199025298608, 0}}, {lock = {tranche = 72, state = {value = 536870912}, 
waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = {head = 
2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {
tranche = 72, state = {value = 536870912}, waiters = {head = 
2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, 
  tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}}, 
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
  value = 536870912}, waiters = {head = 2147483647, tail = 
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 
0, 
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = 
{head = 2147483647, tail = 2147483647}}, spans = {0, 2199025298496, 
2199025298664, 2199025297936}}, {lock = {tranche = 72, state = {
  value = 536870912}, waiters = {head = 2147483647, tail = 
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 
0, 
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = 
{head = 2147483647, tail = 2147483647}}, spans = {0, 8416, 0, 0}}, {lock = 
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647, 
  tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}}, 
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
  value = 536870912}, waiters = {head = 2147483647, tail = 
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 
0, 
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = 
{head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = 
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647, 
  tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}}, 
spans = {0, 8304, 0, 0}}, {lock = {tranche = 72, state = {
  value = 536870912}, waiters = {head = 2147483647, tail = 
2147483647}}, spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {value = 
536870912}, waiters = {head = 2147483647, tail = 2147483647}}, spans = {0, 0, 
0, 
0}}, {lock = {tranche = 72, state = {value = 536870912}, waiters = 
{head = 2147483647, tail = 2147483647}}, spans = {0, 0, 0, 0}}, {lock = 
{tranche = 72, state = {value = 536870912}, waiters = {head = 2147483647, 
  tail = 2147483647}}, spans = {0, 8528, 0, 0}}, {lock = {tranche = 72, 
state = {value = 536870912}, waiters = {head = 2147483647, tail = 2147483647}}, 
spans = {0, 0, 0, 0}}, {lock = {tranche = 72, state = {
  value = 536870912}, 

Re: v15b1: FailedAssertion("segment_map->header->magic == (DSA_SEGMENT_HEADER_MAGIC ^ area->control->handle ^ index)", File: "dsa.c", ..)

[Thomas Munro]

On Wed, Apr 12, 2023 at 11:37 AM Justin Pryzby  wrote:
> $ ls /dev/shm/ |grep 3696856876 || echo not found
> not found

Oh, of course it would have restarted after it crashed and unlinked
that...  So the remaining traces of that memory *might* be in the core
file, depending (IIRC) on the core filter settings (you definitely get
shared anonymous memory like our main shm region by default, but IIRC
there's something extra needed if you want the shm_open'd DSM segments
to be dumped too...)

> (In case it matters: the vm has been up for 1558 days).

I will refrain from invoking cosmic radiation at this point :-)

> If it's helpful, I could provide the corefile, unstripped binaries, and
> libc.so, which would be enough to use gdb on your side with "set
> solib-search-path".

Sounds good, thanks, please send them over off-list and I'll see if I
can figure anything out ...

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Mon, Feb 27, 2023 at 02:33:04PM +, gkokola...@pm.me wrote:
> > > - Finally, the "Nothing to do in the default case" comment comes from
> > > Michael's commit 5e73a6048:
> > > 
> > > + /*
> > > + * Custom and directory formats are compressed by default with gzip when
> > > + * available, not the others.
> > > + /
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > {
> > > #ifdef HAVE_LIBZ
> > > - if (archiveFormat == archCustom || archiveFormat == archDirectory)
> > > - compressLevel = Z_DEFAULT_COMPRESSION;
> > > - else
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > +#else
> > > + / Nothing to do in the default case */
> > > #endif
> > > - compressLevel = 0;
> > > }
> > > 
> > > As the comment says: for -Fc and -Fd, the compression is set to zlib, if
> > > enabled, and when not otherwise specified by the user.
> > > 
> > > Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, and when
> > > zlib was unavailable.
> > > 
> > > But I'm not sure why there's now an empty "#else". I also don't know
> > > what "the default case" refers to.
> > > 
> > > Maybe the best thing here is to move the preprocessor #if, since it's no
> > > longer in the middle of a runtime conditional:
> > > 
> > > #ifdef HAVE_LIBZ
> > > + if ((archiveFormat == archCustom || archiveFormat == archDirectory) &&
> > > + !user_compression_defined)
> > > + parse_compress_specification(PG_COMPRESSION_GZIP, NULL,
> > > + &compression_spec);
> > > #endif
> > > 
> > > ...but that elicits a warning about "variable set but not used"...
> > 
> > 
> > Not sure, I need to think about this a bit.

> /* Nothing to do for the default case when LIBZ is not available */
> is easier to understand.

Maybe I would write it as: "if zlib is unavailable, default to no
compression".  But I think that's best done in the leading comment, and
not inside an empty preprocessor #else.

I was hoping Michael would comment on this.
The placement and phrasing of the comment makes no sense to me.

-- 
Justin