Re: PostgreSQL 16 release announcement draft

[Erik Rijkers]

Op 8/26/23 om 04:51 schreef Jonathan S. Katz:

On 8/24/23 11:17 AM, Erik Rijkers wrote:

Op 8/24/23 om 16:32 schreef Jonathan S. Katz:

On 8/23/23 5:07 PM, David Rowley wrote:
On Thu, 24 Aug 2023 at 05:55, Jonathan S. Katz 
 wrote:


Hi,

When v15 docs have:

"27.2.7. Cascading Replication
The cascading replication feature allows a standby server to accept 
replication connections and stream WAL records to other standbys, 
acting as a relay. This can be used to reduce the number of direct 
connections to the primary and also to minimize inter-site bandwidth 
overheads."


why then, in the release draft, is that capability mentioned as 
something that is new for v16?

"
In PostgreSQL 16, users can perform logical decoding from a standby
instance, meaning a standby can publish logical changes to other servers.
"

Is there a difference between the two?


Yes. Those docs refer to **physical** replication, where a standby can 
continue to replicate WAL records to other standbys. In v16, standbys 
can now publish changes over **logical** replication.


Well, I must assume you are right.

But why is the attached program, running 3 cascading v15 servers, 
showing 'logical' in the middle server's (port 6526) 
pg_replication_slots.slot_type ?  Surely that is not physical but 
logical replication?


 port |  svn   | slot_name  | slot_type
--+++---
 6526 | 150003 | pub_6527_from_6526 | logical   <--
(1 row)

I must be confused -- I will be thankful for enlightenment.

Erik


Thanks,

Jonathan


logrep_cascade_15.sh
Description: application/shellscript

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

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thank you for reviewing! The patch can be available in [1].

> Here are my review comments for patch v25-0003.
> 
> ==
> src/bin/pg_upgrade/check.c
> 
> 1. GENERAL
> 
> +static void check_for_confirmed_flush_lsn(void);
> +static void check_for_lost_slots(void);
> 
> For more clarity, I wonder if it is better to rename some functions:
> 
> check_for_confirmed_flush_lsn() -> check_old_cluster_for_confirmed_flush_lsn()
> check_for_lost_slots() -> check_old_cluster_for_lost_slots()

Replaced.

> 2.
> + /*
> + * Logical replication slots can be migrated since PG17. See comments atop
> + * get_logical_slot_infos().
> + */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> + {
> + check_for_lost_slots();
> +
> + /*
> + * Do additional checks if a live check is not required. This requires
> + * that confirmed_flush_lsn of all the slots is the same as the latest
> + * checkpoint location, but it would be satisfied only when the server
> + * has been shut down.
> + */
> + if (!live_check)
> + check_for_confirmed_flush_lsn();
> + }
> +
> 
> 2a.
> If my suggestions from v25-0002 [1] are adopted then this comment
> needs to change to say like "See atop file pg_upgrade.c..."
>
> 2b.
> Hmm. If my suggestions from v25-0002 [1] are adopted then the version
> checking and the slot counting would *already* be in this calling
> function. In that case, why can't this whole fragment be put in the
> same place? E.g. IIUC there is no reason to call these at checks all
> when the old_cluster slot count is already known to be 0. Similarly,
> there is no reason that both these functions need to be independently
> checking count_logical_slots again since we have already done that
> (again, assuming my suggestions from v25-0002 [1] are adopted).

Currently I did not accept the comment, so they were ignored.

> 3. check_for_lost_slots
> 
> +/*
> + * Verify that all logical replication slots are usable.
> + */
> +void
> +check_for_lost_slots(void)
> 
> This was forward-declared to be static, but the static function
> modifier is absent here.

Fixed.

> 4. check_for_lost_slots
> 
> + /* Quick exit if the cluster does not have logical slots. */
> + if (count_logical_slots() == 0)
> + return;
> +
> 
> AFAICT this quick exit can be removed. See my comment #2b.

2b was skipped, so IIUC this is still needed.

> 5. check_for_confirmed_flush_lsn
> 
> +check_for_confirmed_flush_lsn(void)
> +{
> + int i,
> + ntups,
> + i_slotname;
> + PGresult   *res;
> + DbInfo*active_db = _cluster.dbarr.dbs[0];
> + PGconn*conn;
> +
> + /* Quick exit if the cluster does not have logical slots. */
> + if (count_logical_slots() == 0)
> + return;
> 
> AFAICT this quick exit can be removed. See my comment #2b.

I kept the style.

> .../t/003_logical_replication_slots.pl
> 
> 6.
> +# 2. Temporarily disable the subscription
> +$subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub DISABLE");
>  $old_publisher->stop;
> 
> In my previous 0003 review ([2] #10b) I was not questioning the need
> for the $old_publisher->stop; before the pg_upgrade. I was only asking
> why it was done at this location (after the DISABLE) instead of
> earlier.

I see. The reason was to avoid unnecessary error by apply worker.

As the premise, the position of shutting down (before or after the DISABLE) does
not affect the result. But if it puts earlier than DISABLE, the apply worker 
will
exit with below error because the walsender exits ealier than worker:

```
ERROR:  could not send end-of-streaming message to primary: server closed the 
connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
no COPY in progress
```

It is not problematic but future readers may be confused if it find.
So I avoided it.

> 7.
> +# Check whether changes on the new publisher get replicated to the subscriber
> +$new_publisher->safe_psql('postgres',
> + "INSERT INTO tbl VALUES (generate_series(11, 20))");
> +$new_publisher->wait_for_catchup('sub');
> +$result = $subscriber->safe_psql('postgres', "SELECT count(*) FROM tbl");
> +is($result, qq(20), 'check changes are shipped to the subscriber');
> 
> /shipped/replicated/

You meant to say s/replicated/shipped/, right? Fixed.

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thank you for reviewing! PSA new version patch set.
 
> ==
> Commit message.
> 
> 1.
> I felt this should mention the limitation that the slot upgrade
> feature is only supported from PG17 slots upwards.

Added. The same sentence as doc was used.

> doc/src/sgml/ref/pgupgrade.sgml
> 
> 2.
> +
> + pg_upgrade attempts to migrate logical
> + replication slots. This helps avoid the need for manually defining the
> + same replication slots on the new publisher. Currently,
> + pg_upgrade supports migrate logical
> replication
> + slots when the old cluster is 17.X and later.
> +
> 
> Currently, pg_upgrade supports migrate
> logical replication slots when the old cluster is 17.X and later.
> 
> SUGGESTION
> Migration of logical replication slots is only supported when the old
> cluster is version 17.0 or later.

Fixed.

> src/bin/pg_upgrade/check.c
> 
> 3. GENERAL
> 
> IMO all version checking for this feature should only be done within
> this "check.c" file as much as possible.
> 
> The detailed reason for this PG17 limitation can be in the file header
> comment of "pg_upgrade.c", and then all the version checks can simply
> say something like:
> "Logical slot migration is only support for slots in PostgreSQL 17.0
> and later. See atop file pg_upgrade.c for an explanation of this
> limitation "

Hmm, I'm not sure it should be and Amit disagreed [1].
I did not address this one.

> 4. check_and_dump_old_cluster
> 
> + /* Extract a list of logical replication slots */
> + get_logical_slot_infos();
> +
> 
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the within
> get_logical_slot_infos() and put here in the caller.
> 
> SUGGESTION
> 
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> /* Extract a list of logical replication slots */
> get_logical_slot_infos();
> }

Per discussion [1], I did not address the comment.

> 5. check_new_cluster_logical_replication_slots
> 
> +check_new_cluster_logical_replication_slots(void)
> +{
> + PGresult   *res;
> + PGconn*conn;
> + int nslots = count_logical_slots();
> + int max_replication_slots;
> + char*wal_level;
> +
> + /* Quick exit if there are no logical slots on the old cluster */
> + if (nslots == 0)
> + return;
> 
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the count_logical_slots() and
> then this code should be written more like this:
> 
> SUGGESTION (notice the quick return comment change too)
> 
> int nslots = 0;
> 
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> nslots = count_logical_slots();
> 
> /* Quick return if there are no logical slots to be migrated. */
> if (nslots == 0)
> return;

Fixed.

> src/bin/pg_upgrade/info.c
> 
> 6. GENERAL
> 
> For the sake of readability it might be better to make the function
> names more explicit:
> 
> get_logical_slot_infos() -> get_old_cluster_logical_slot_infos()
> count_logical_slots() -> count_old_cluster_logical_slots()

Fixed. Moreover, get_logical_slot_infos_per_db() also followed the style.

> 7. get_logical_slot_infos
> 
> +/*
> + * get_logical_slot_infos()
> + *
> + * Higher level routine to generate LogicalSlotInfoArr for all databases.
> + *
> + * Note: This function will not do anything if the old cluster is pre-PG 17.
> + * The logical slots are not saved at shutdown, and the confirmed_flush_lsn 
> is
> + * always behind the SHUTDOWN_CHECKPOINT record. Subsequent checks
> done in
> + * check_for_confirmed_flush_lsn() would raise a FATAL error if such slots 
> are
> + * included.
> + */
> +void
> +get_logical_slot_infos(void)
> 
> Move all this detailed explanation about the limitation to the
> file-level comment in "pg_upgrade.c". See also review comment #3.

Per discussion [1], I did not address the comment.

> 8. get_logical_slot_infos
> 
> +void
> +get_logical_slot_infos(void)
> +{
> + int dbnum;
> +
> + /* Logical slots can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return;
> 
> IMO the version checking is best done in the "checking" functions. See
> previous review comments about the caller of this. If you want to put
> something here, then just have an Assert:
> 
> Assert(GET_MAJOR_VERSION(old_cluster.major_version) >= 1700);

As I said above, check_and_dump_old_cluster() still does not check major version
before calling get_old_cluster_logical_slot_infos(). So I kept current style.

> 9. count_logical_slots
> 
> +/*
> + * count_logical_slots()
> + *
> + * Sum up and return the number of logical replication slots for all 
> databases.
> + */
> +int
> +count_logical_slots(void)
> +{
> + int dbnum;
> + int slot_count = 0;
> +
> + /* Quick exit if the version is prior to PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return 

Re: PSQL error: total cell count of XXX exceeded

[David G. Johnston]

On Friday, August 25, 2023, Hongxu Ma  wrote:
>
>
> When I tried to select a big amount of rows, psql complains a error "Cannot
> add cell to table content: total cell count of 905032704 exceeded."
>
>  We should use long for ncolumns and nrows and give a more obvious error
> message here.
>
> Any thoughts? or some other hidden reasons?
>

9 millions cells seems more than realistic a limit for a psql query result
output.  In any case it isn’t a bug, the code demonstrates that fact by
producing an explicit error.

I wouldn’t be adverse to an improved error message, and possibly
documenting said limit.

David J.

Re: PostgreSQL 16 release announcement draft

[Jonathan S. Katz]

On 8/24/23 11:19 AM, Alvaro Herrera wrote:

On 2023-Aug-24, Jonathan S. Katz wrote:


### Performance Improvements

PostgreSQL 16 improves the performance of existing PostgreSQL functionality
through new query planner optimizations. In this latest release, the query
planner can parallelize  `FULL` and `RIGHT` joins, generate better optimized
plans for queries that use aggregate functions (e.g. `count`) with a `DISTINCT`
or `ORDER BY` clause, utilize incremental sorts for `SELECT DISTINCT` queries,
and optimize window function executions so they execute more efficiently.


"optimize window function executions so that they execute blah" sounds
redundant and strange. Maybe just "optimize execution of window
functions" is sufficient?  Also, using "e.g." there looks somewhat out
of place; maybe "(such as `count`)" is a good replacement?


It also introduces `RIGHT` and `OUTER` "anti-joins", which enable users to
identify rows not present in a joined table.


Wait.  Are you saying we didn't have those already?  Looking at
release-16.sgml I think this refers to commit 16dc2703c541, which means
this made them more efficient rather than invented them.



This release includes improvements for bulk loading using `COPY` in both single
and concurrent operations, with tests showing up to a 300% performance
improvement in some cases. PostgreSQL adds support for load balancing in clients


PostgreSQL 16


that use `libpq`, and improvements to vacuum strategy that reduce the necessity
of full-table freezes. Additionally, PostgreSQL 16 introduces CPU acceleration
using `SIMD` in both x86 and ARM architectures, resulting in performance gains
when processing ASCII and JSON strings, and performing array and subtransaction
searches.

### Logical replication

Logical replication lets PostgreSQL users stream data to other PostgreSQL


"L.R. in PostgreSQL lets users"?


instances or subscribers that can interpret the PostgreSQL logical replication
protocol. In PostgreSQL 16, users can perform logical decoding from a standby


s/decoding/replication/ ? (It seems odd to use "decoding" when the
previous sentence used "replication")


instance, meaning a standby can publish logical changes to other servers. This
provides developers with new workload distribution options – for example, using
a standby rather than the busier primary to logically replicate changes to
downstream systems.

Additionally, there are several performance improvements in PostgreSQL 16 to
logical replication. Subscribers can now apply large transactions using parallel
workers. For tables that do not have a `PRIMARY KEY`, subscribers can use B-tree


"a primary key", no caps.


indexes instead of sequential scans to find rows. Under certain conditions,
users can also speed up initial table synchronization using the binary format.

There are several access control improvements to logical replication in
PostgreSQL 16, including the new predefined role pg_create_subscription, which
grants users the ability to create a new logical subscription. Finally, this
release begins adding support for bidirectional logical replication, introducing
functionality to replicate data between two tables from different publishers.


"to create a new logical subscription" -> "to create new logical subscriptions"


### Developer Experience

PostgreSQL 16 adds more syntax from the SQL/JSON standard, including
constructors and predicates such as `JSON_ARRAY()`, `JSON_ARRAYAGG()`, and
`IS JSON`. This release also introduces the ability to use underscores for
thousands separators (e.g. `5_432_000`) and non-decimal integer literals, such
as `0x1538`, `0o12470`, and `0b1010100111000`.

Developers using PostgreSQL 16 will also benefit from the addition of multiple
commands to `psql` client protocol, including the `\bind` command, which allows
users to execute parameterized queries (e.g `SELECT $1 + $2`) then use `\bind`
to substitute the variables.


This paragraph sounds a bit suspicious.  What do you mean with "multiple
commands to psql client protocol"?  Also, I think "to execute parameterized
queries" should be "to prepare parameterized queries", and later "then
use \bind to execute the query substituting the variables".




### Monitoring

A key aspect of tuning the performance of database workloads is understanding
the impact of your I/O operations on your system. PostgreSQL 16 helps simplify
how you can analyze this data with the new pg_stat_io view, which tracks key I/O
statistics such as shared_buffer hits and I/O latency.


Hmm, I think what pg_stat_io gives you is data which wasn't available
previously at all.  Maybe do something like "Pg 16 introduces
pg_stat_io, a new source of key I/O metrics that can be used for more
fine grained something something".


Additionally, this release adds a new field to the `pg_stat_all_tables` view
that records a timestamp representing when a table or index was last scanned.
PostgreSQL also makes auto_explain more readable by logging values 

PSQL error: total cell count of XXX exceeded

[Hongxu Ma]

Hi Hackers,

When I tried to select a big amount of rows, psql complains a error "Cannot add 
cell to table content: total cell count of 905032704 exceeded."

Here are the reproduce steps:
```
interma=# select version();
 version
-
 PostgreSQL 12.13 on x86_64-pc-linux-gnu, compiled by gcc (Debian 8.3.0-6) 
8.3.0, 64-bit
(1 row)

interma=# create table t26(a int,b int,c int,d int,e int,f int,g int,h int,i 
int,j int,k int,l int,m int,n int,o int,p int,q int,r int,s int,t int ,u int,v 
int,w int,x int,y int,z int);
CREATE TABLE
interma=# insert into t26 select generate_series(1,2);
INSERT 0 2
interma=# select * from t26;
Cannot add cell to table content: total cell count of 905032704 exceeded.
```

I checked the related code, and root cause is clear:
```
// in printTableAddCell()
if (content->cellsadded >= content->ncolumns * content->nrows)
  report this error and exit

// cellsadded is long type, but ncolumns and nrows are int
// so, it's possible overflow the int value here.

// using a test program to verify:
int rows = 2;
int cols = 26;
printf("%d*%d = %d\n", rows,cols, rows*cols);

output:
  2,,*26 = 9,0503,2704 // overflow and be truncated into int value 
here
```

Based on it, I think it's a bug. We should use long for ncolumns and nrows and 
give a more obvious error message here.

My version is 12.13, and I think the latest code also exists this issue: issue: 
https://github.com/postgres/postgres/blob/1a4fd77db85abac63e178506335aee74625f6499/src/fe_utils/print.c#L3259

Any thoughts? or some other hidden reasons?
Thanks.

Re: PostgreSQL 16 release announcement draft

[Jonathan S. Katz]

On 8/24/23 11:17 AM, Erik Rijkers wrote:

Op 8/24/23 om 16:32 schreef Jonathan S. Katz:

On 8/23/23 5:07 PM, David Rowley wrote:
On Thu, 24 Aug 2023 at 05:55, Jonathan S. Katz  
wrote:


Hi,

When v15 docs have:

"27.2.7. Cascading Replication
The cascading replication feature allows a standby server to accept 
replication connections and stream WAL records to other standbys, acting 
as a relay. This can be used to reduce the number of direct connections 
to the primary and also to minimize inter-site bandwidth overheads."


why then, in the release draft, is that capability mentioned as 
something that is new for v16?

"
In PostgreSQL 16, users can perform logical decoding from a standby
instance, meaning a standby can publish logical changes to other servers.
"

Is there a difference between the two?


Yes. Those docs refer to **physical** replication, where a standby can 
continue to replicate WAL records to other standbys. In v16, standbys 
can now publish changes over **logical** replication.


Thanks,

Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: PostgreSQL 16 release announcement draft

[Jonathan S. Katz]

On 8/24/23 12:54 PM, Dave Cramer wrote:




 > Postgres, PostgreSQL, and the Elephant Logo (Slonik) are all
registered
 > trademarks of the [PostgreSQL Community Association of
Canada](https://www.postgres.ca ).

Isn't this just the "PostgreSQL Community Association", no Canada?


Certainly confusing from the website, but in the about section is this
"PostgreSQL Community Association is a trade or business name of the 
PostgreSQL Community Association of Canada."


This was something I missed when reviewing the fulltext, and went ahead 
and fixed it. Thanks,


Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: pg_stat_get_backend_subxact() and backend IDs?

[Imseih (AWS), Sami]

> Here is a new version of the patch that avoids changing the names of the
> existing functions. I'm not thrilled about the name
> (pgstat_fetch_stat_local_beentry_by_backend_id), so I am open to
> suggestions. In any case, I'd like to rename all three of the>
> pgstat_fetch_stat_* functions in v17.

Thanks for the updated patch.

I reviewed/tested the latest version and I don't have any more comments.

Regards,

Sami

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[Cary Huang]

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

Hello

Thank you for the patch and the effort to enhance \d+ 's output on partitioned 
tables that contain sub-partitions. However, the patch does not apply and I 
notice that this patch is generated as a differ file from 2 files, describe.c 
and describe_change.c. You should use git diff to generate a patch rather than 
maintaining 2 files yourself. Also I noticed that you include a 
"create_object.sql" file to illustrate the feature, which is not necessary. 
Instead, you should add them as a regression test cases in the existing 
regression test suite under "src/test/regress", so these will get run as tests 
to illustrate the feature. This patch changes the output of \d+ and it could 
potentially break other test cases so you should fix them in the patch in 
addition to providing the feature

Now, regarding the feature, I see that you intent to print the sub partitions' 
partitions in the output, which is okay in my opinion. However, a sub-partition 
can also contain another sub-partition, which contains another sub-partition 
and so on. So it is possible that sub-partitions can span very, very deep. Your 
example assumes only 1 level of sub-partitions. Are you going to print all of 
them out in \d+? If so, it would definitely cluster the output so much that it 
starts to become annoying. Are you planning to set a limit on how many levels 
of sub-partitions to print or just let it print as many as it needs?

thank you

Cary Huang
---
Highgo Software Canada
www.highgo.ca

Re: Make all Perl warnings fatal

[Dagfinn Ilmari Mannsåker]

Alvaro Herrera  writes:

> On 2023-Aug-10, Peter Eisentraut wrote:
>
>> I wanted to figure put if we can catch these more reliably, in the style of
>> -Werror.  AFAICT, there is no way to automatically turn all warnings into
>> fatal errors.  But there is a way to do it per script, by replacing
>> 
>> use warnings;
>> 
>> by
>> 
>> use warnings FATAL => 'all';
>> 
>> See attached patch to try it out.
>
> BTW in case we do find that there's some unforeseen problem and we want
> to roll back, it would be great to have a way to disable this without
> having to edit every single Perl file again later.  However, I didn't
> find a way to do it -- I thought about creating a separate PgWarnings.pm
> file that would do the "use warnings FATAL => 'all'" dance and which
> every other Perl file would use or include; but couldn't make it work.
> Maybe some Perl expert knows a good answer to this.

Like most pragmas (all-lower-case module names), `warnings` affects the
currently-compiling lexical scope, so to have a module like PgWarnings
inject it into the module that uses it, you'd call warnings->import in
its import method (which gets called when the `use PgWarnings;``
statement is compiled, e.g.:

package PgWarnings;

sub import {
warnings->import(FATAL => 'all');
}

I wouldn't bother with a whole module just for that, but if we have a
group of pragmas or modules we always want to enable/import and have the
ability to change this set without having to edit all the files, it's
quite common to have a ProjectName::Policy module that does that.  For
example, to exclude warnings that are unsafe, pointless, or impossible
to fatalise (c.f. https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS):

package PostgreSQL::Policy;

sub import {
strict->import;
warnings->import(
FATAL => 'all',
NONFATAL => qw(exec internal malloc recursion),
);
warnings->uniport(qw(once));
}

Now that we require Perl 5.14, we might want to consider enabling its
feature bundle as well, with:

feature->import(':5.14')

Although the only features of note that adds are:

   - say: the `say` function, like `print` but appends a newline

   - state: `state` variables, like `my` but only initialised the first
 time the function they're in is called, and the value persists
 between calls (like function-scoped `static` variables in C)

   - unicode_strings: use unicode semantics for characters in the
 128-255 range, regardless of internal representation

> Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that
> emits the "use warnings" line?

That's ugly as sin, and thankfully not necessary.

-ilmari

Re: pg_stat_get_backend_subxact() and backend IDs?

[Nathan Bossart]

On Fri, Aug 25, 2023 at 08:32:51AM -0700, Nathan Bossart wrote:
> On second thought, renaming these exported functions so close to release is
> probably not a great idea.  I should probably skip back-patching that one.
> Or I could have the existing functions call the new ones in v16 for
> backward compatibility...

Here is a new version of the patch that avoids changing the names of the
existing functions.  I'm not thrilled about the name
(pgstat_fetch_stat_local_beentry_by_backend_id), so I am open to
suggestions.  In any case, I'd like to rename all three of the
pgstat_fetch_stat_* functions in v17.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From e1ce8bf67f1b713294ef8d38dbee26bfc7ef16f4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Thu, 24 Aug 2023 07:58:01 -0700
Subject: [PATCH v3 1/1] fix pg_stat_get_backend_subxact to use real backend id

---
 src/backend/utils/activity/backend_status.c | 39 -
 src/backend/utils/adt/pgstatfuncs.c |  2 +-
 src/include/utils/backend_status.h  |  1 +
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 38f91a495b..231cc5dd9a 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -1089,9 +1089,34 @@ cmp_lbestatus(const void *a, const void *b)
 PgBackendStatus *
 pgstat_fetch_stat_beentry(BackendId beid)
 {
-	LocalPgBackendStatus key;
 	LocalPgBackendStatus *ret;
 
+	ret = pgstat_fetch_stat_local_beentry_by_backend_id(beid);
+	if (ret)
+		return >backendStatus;
+
+	return NULL;
+}
+
+
+/* --
+ * pgstat_fetch_stat_local_beentry_by_backend_id() -
+ *
+ *	Like pgstat_fetch_stat_beentry() but with locally computed additions
+ *	(like xid and xmin values of the backend)
+ *
+ *	The beid argument is the BackendId of the desired session
+ *	(note that this is unlike pgstat_fetch_stat_local_beentry()).
+ *
+ *	NB: caller is responsible for checking if the user is permitted to see this
+ *	info (especially the querystring).
+ * --
+ */
+LocalPgBackendStatus *
+pgstat_fetch_stat_local_beentry_by_backend_id(BackendId beid)
+{
+	LocalPgBackendStatus key;
+
 	pgstat_read_current_status();
 
 	/*
@@ -1099,14 +1124,10 @@ pgstat_fetch_stat_beentry(BackendId beid)
 	 * bsearch() to search it efficiently.
 	 */
 	key.backend_id = beid;
-	ret = (LocalPgBackendStatus *) bsearch(, localBackendStatusTable,
-		   localNumBackends,
-		   sizeof(LocalPgBackendStatus),
-		   cmp_lbestatus);
-	if (ret)
-		return >backendStatus;
-
-	return NULL;
+	return (LocalPgBackendStatus *) bsearch(, localBackendStatusTable,
+			localNumBackends,
+			sizeof(LocalPgBackendStatus),
+			cmp_lbestatus);
 }
 
 
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 2b9742ad21..68ae044399 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -727,7 +727,7 @@ pg_stat_get_backend_subxact(PG_FUNCTION_ARGS)
 
 	BlessTupleDesc(tupdesc);
 
-	if ((local_beentry = pgstat_fetch_stat_local_beentry(beid)) != NULL)
+	if ((local_beentry = pgstat_fetch_stat_local_beentry_by_backend_id(beid)) != NULL)
 	{
 		/* Fill values and NULLs */
 		values[0] = Int32GetDatum(local_beentry->backend_subxact_count);
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 77939a0aed..060431a39b 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -335,6 +335,7 @@ extern uint64 pgstat_get_my_query_id(void);
 extern int	pgstat_fetch_stat_numbackends(void);
 extern PgBackendStatus *pgstat_fetch_stat_beentry(BackendId beid);
 extern LocalPgBackendStatus *pgstat_fetch_stat_local_beentry(int beid);
+extern LocalPgBackendStatus *pgstat_fetch_stat_local_beentry_by_backend_id(BackendId beid);
 extern char *pgstat_clip_activity(const char *raw_activity);
 
 
-- 
2.25.1

Support prepared statement invalidation when result types change

[Jelte Fennema]

The cached plan for a prepared statements can get invalidated when DDL
changes the tables used in the query, or when search_path changes. When
this happens the prepared statement can still be executed, but it will
be replanned in the new context. This means that the prepared statement
will do something different e.g. in case of search_path changes it will
select data from a completely different table. This won't throw an
error, because it is considered the responsibility of the operator and
query writers that the query will still do the intended thing.

However, we would throw an error if the the result of the query is of a
different type than it was before:
ERROR: cached plan must not change result type

This requirement was not documented anywhere and it
can thus be a surprising error to hit. But it's actually not needed for
this to be an error, as long as we send the correct RowDescription there
does not have to be a problem for clients when the result types or
column counts change.

This patch starts to allow a prepared statement to continue to work even
when the result type changes.

Without this change all clients that automatically prepare queries as a
performance optimization will need to handle or avoid the error somehow,
often resulting in deallocating and re-preparing queries when its
usually not necessary. With this change connection poolers can also
safely prepare the same query only once on a connection and share this
one prepared query across clients that prepared that exact same query.

Some relevant previous discussions:
[1]: 
https://www.postgresql.org/message-id/flat/CAB%3DJe-GQOW7kU9Hn3AqP1vhaZg_wE9Lz6F4jSp-7cm9_M6DyVA%40mail.gmail.com
[2]: 
https://stackoverflow.com/questions/2783813/postgres-error-cached-plan-must-not-change-result-type
[3]: 
https://stackoverflow.com/questions/42119365/how-to-avoid-cached-plan-must-not-change-result-type-error
[4]: https://github.com/pgjdbc/pgjdbc/pull/451
[5]: https://github.com/pgbouncer/pgbouncer/pull/845#discussion_r1305295551
[6]: https://github.com/jackc/pgx/issues/927
[7]: 
https://elixirforum.com/t/postgrex-errors-with-cached-plan-must-not-change-result-type-during-migration/51235/2
[8]: https://github.com/rails/rails/issues/12330
From b1a58082f2b226b37d237580e33b52438d480f48 Mon Sep 17 00:00:00 2001
From: Jelte Fennema 
Date: Fri, 25 Aug 2023 17:09:38 +0200
Subject: [PATCH v1 1/2] Support prepared statement invalidation when result
 types change

The cached plan for a prepared statements can get invalidated when DDL
changes the tables used in the query, or when search_path changes. When
this happens the prepared statement can still be executed, but it will
be replanned in the new context. This means that the prepared statement
will do something different e.g. in case of search_path changes it will
select data from a completely different table. This won't throw an
error, because it is considered the responsibility of the operator and
query writers that the query will still do the intended thing.

However, we would throw an error if the the result of the query is of a
different type than it was before. This was not documented anywhere and
can thus be a surprising error to hit. But it's actually not needed for
this to be an error, as long as we send the correct RowDescription there
does not have to be a problem for clients when the result types or
column counts change.

This patch starts to allow a prepared statement to continue to work even
when the result type changes.

Without this change all clients that automatically prepare queries as a
performance optimization will need to handle or avoid the error somehow,
often resulting in deallocating and re-preparing queries when its
usually not necessary. With this change connection poolers can also
safely prepare the same query only once on a connection and share this
one prepared query across clients that prepared that exact same query.
---
 src/backend/tcop/pquery.c   | 46 +++--
 src/backend/utils/cache/plancache.c |  5 ---
 src/test/regress/expected/plancache.out | 24 +
 src/test/regress/sql/plancache.sql  |  7 ++--
 4 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3d..ee790009cd2 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -561,23 +561,21 @@ PortalStart(Portal portal, ParamListInfo params,
 
 			case PORTAL_UTIL_SELECT:
 
-/*
- * We don't set snapshot here, because PortalRunUtility will
- * take care of it if needed.
- */
-{
-	PlannedStmt *pstmt = PortalGetPrimaryStmt(portal);
-
-	Assert(pstmt->commandType == CMD_UTILITY);
-	portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt);
-}
-
 /*
  * Reset cursor position data to "start of query"
  */
 portal->atStart = true;
 portal->atEnd = false;	/* allow fetches */
 portal->portalPos = 0;
+
+/*
+ * The tupDesc 

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Vik Fearing]

On 8/25/23 17:56, Chapman Flack wrote:

[0] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards


I was not aware of this page.  What a wealth of information!
--
Vik Fearing

Re: initdb caching during tests

[Nathan Bossart]

On Thu, Aug 24, 2023 at 03:10:00PM -0700, Andres Freund wrote:
> Cool. Pushed that way.

I just noticed the tests running about 30% faster on my machine due to
this.  Thanks!

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

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Chapman Flack]

On 2023-08-25 10:49, Vik Fearing wrote:

I do not think this should be addressed in this patch because
there are quite a lot of functions that need to handle this.


Indeed, as described in [0], we still largely provide the SQL/XML:2003
notion of a single XML datatype, not the distinguishable XML(DOCUMENT),
XML(CONTENT), XML(SEQUENCE) types from :2006 and later, which has a
number of adverse consequences for developers[1], and that wiki page
proposed a couple possible ways forward[2].

Regards,
-Chap


[0] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards
[1] 
https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards#Obstacles_to_improving_conformance
[2] 
https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards#Possible_ways_forward

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Jim Jones]

On 25.08.23 16:49, Vik Fearing wrote:


I am talking specifically about this:

@@ -505,6 +506,10 @@ xmlcomment(PG_FUNCTION_ARGS)
 appendStringInfoText(, arg);
 appendStringInfoString(, "-->");

+
+
+
+
 PG_RETURN_XML_P(stringinfo_to_xmltype());
 #else
 NO_XML_SUPPORT();


I have no idea how xmlcomment() got changed in this patch :D nice catch!



I do not think this should be addressed in this patch because there 
are quite a lot of functions that need to handle this.


v4 attached.

Jim
From ea812791ffdce22d5f5615da361f67c801089f91 Mon Sep 17 00:00:00 2001
From: Jim Jones 
Date: Fri, 25 Aug 2023 14:05:39 +0200
Subject: [PATCH v4] Add XMLText function (SQL/XML X038)

This function implements the standard XMLTest function, which
converts text into xml text nodes. It uses the libxml2 function
xmlEncodeSpecialChars to escape predifined entites (&"<>), so
that those do not cause any conflict when concatenating the text
node output with existing xml documents.

This patch includes also documentation and regression tests.
---
 doc/src/sgml/func.sgml   | 30 +++
 src/backend/catalog/sql_features.txt |  2 +-
 src/backend/utils/adt/xml.c  | 22 +
 src/include/catalog/pg_proc.dat  |  3 +++
 src/test/regress/expected/xml.out| 36 
 src/test/regress/expected/xml_1.out  | 23 ++
 src/test/regress/expected/xml_2.out  | 36 
 src/test/regress/sql/xml.sql |  7 ++
 8 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7a0d4b9134..2f01a2c25d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14058,6 +14058,36 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
 documents for processing in client applications.

 
+  
+xmltext
+
+
+ xmltext
+
+
+
+xmltext ( text ) xml
+
+
+
+ The function xmltext returns an XML value with a single
+ text node containing the input argument as its content. Predefined entities
+ like ampersand (), left and right angle brackets
+ (), and quotation marks ()
+ are escaped.
+
+
+
+ Example:
+
+
+   
+

 xmlcomment
 
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index b33065d7bf..680d541673 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -633,7 +633,7 @@ X034	XMLAgg			YES
 X035	XMLAgg: ORDER BY option			YES	
 X036	XMLComment			YES	
 X037	XMLPI			YES	
-X038	XMLText			NO	
+X038	XMLText			YES	
 X040	Basic table mapping			YES	
 X041	Basic table mapping: null absent			YES	
 X042	Basic table mapping: null as nil			YES	
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 866d0d649a..7f6984dd88 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -47,6 +47,7 @@
 
 #ifdef USE_LIBXML
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -5006,3 +5007,24 @@ XmlTableDestroyOpaque(TableFuncScanState *state)
 	NO_XML_SUPPORT();
 #endif			/* not USE_LIBXML */
 }
+
+Datum
+xmltext(PG_FUNCTION_ARGS)
+{
+#ifdef USE_LIBXML
+	text	   *arg = PG_GETARG_TEXT_PP(0);
+	text 	   *result;
+	xmlChar*xmlbuf = NULL;
+
+	xmlbuf = xmlEncodeSpecialChars(NULL,xml_text2xmlChar(arg));
+
+	Assert(xmlbuf);
+
+	result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
+	xmlFree(xmlbuf);
+	PG_RETURN_XML_P(result);
+#else
+	NO_XML_SUPPORT();
+	return 0;
+#endif
+}
\ No newline at end of file
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..ff00c6365d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8789,6 +8789,9 @@
 { oid => '2922', descr => 'serialize an XML value to a character string',
   proname => 'text', prorettype => 'text', proargtypes => 'xml',
   prosrc => 'xmltotext' },
+{ oid => '3813', descr => 'generate XML text node',
+  proname => 'xmltext', proisstrict => 't', prorettype => 'xml',
+  proargtypes => 'text', prosrc => 'xmltext' },
 
 { oid => '2923', descr => 'map table contents to XML',
   proname => 'table_to_xml', procost => '100', provolatile => 's',
diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out
index 398345ca67..13e4296bf8 100644
--- a/src/test/regress/expected/xml.out
+++ b/src/test/regress/expected/xml.out
@@ -1785,3 +1785,39 @@ SELECT * FROM XMLTABLE('.' PASSING XMLELEMENT(NAME a) columns a varchar(20) PATH
   | foo/
 (1 row)
 
+SELECT xmltext(NULL);
+ xmltext 
+-
+ 
+(1 row)
+
+SELECT xmltext('');
+ xmltext 
+-
+ 
+(1 row)
+
+SELECT xmltext('  ');
+ xmltext 
+-
+   
+(1 row)
+
+SELECT xmltext('foo `$_-+?=*^%!|/\()[]{}');
+ xmltext  
+--
+ foo `$_-+?=*^%!|/\()[]{}
+(1 row)
+
+SELECT xmltext('foo & <"bar">');
+  

Re: pg_stat_get_backend_subxact() and backend IDs?

[Nathan Bossart]

On Fri, Aug 25, 2023 at 03:01:40PM +, Imseih (AWS), Sami wrote:
> 1/ cast the return of bsearch. This was done previously and is the common
> convention in the code.

Will do.

> 2/ This will probably be a good time to update the docs for 
> pg_stat_get_backend_subxact [1]
> to call out that "subxact_count" will "only increase if a transaction is 
> performing writes". Also to link
> the reader to the subtransactions doc [2].

I'd rather keep this patch focused on fixing the bug, given we are so close
to the v16 release.

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

Re: pg_stat_get_backend_subxact() and backend IDs?

[Nathan Bossart]

On Fri, Aug 25, 2023 at 09:36:18AM +0900, Ian Lawrence Barwick wrote:
> Thanks for looking at this. In summary we now have these functions:
> 
>  extern PgBackendStatus  *pgstat_get_beentry_by_backend_id(BackendId 
> beid);
>  extern LocalPgBackendStatus
> *pgstat_get_local_beentry_by_backend_id(BackendId beid);
>  extern LocalPgBackendStatus *pgstat_get_local_beentry_by_index(int beid);
> 
> which LGTM; patches work as expected and resolve the reported issue.

On second thought, renaming these exported functions so close to release is
probably not a great idea.  I should probably skip back-patching that one.
Or I could have the existing functions call the new ones in v16 for
backward compatibility...

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

Re: pg_stat_get_backend_subxact() and backend IDs?

[Imseih (AWS), Sami]

I tested the patch and it does the correct thing.

I have a few comments:

1/ cast the return of bsearch. This was done previously and is the common
convention in the code.

So

+   return bsearch(, localBackendStatusTable, localNumBackends,
+  sizeof(LocalPgBackendStatus), cmp_lbestatus);

Should be

+   return (LocalPgBackendStatus *) bsearch(, localBackendStatusTable, 
localNumBackends,
+  sizeof(LocalPgBackendStatus), cmp_lbestatus);

2/ This will probably be a good time to update the docs for 
pg_stat_get_backend_subxact [1]
to call out that "subxact_count" will "only increase if a transaction is 
performing writes". Also to link
the reader to the subtransactions doc [2].


1. 
https://www.postgresql.org/docs/16/monitoring-stats.html#WAIT-EVENT-TIMEOUT-TABLE
2. https://www.postgresql.org/docs/16/subxacts.html


Regards,

Sami Imseih
Amazon Web Services (AWS)

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Vik Fearing]

On 8/25/23 14:42, Jim Jones wrote:

Hi Vik

Thanks for reviewing my patch!


Thank you for writing it!


On 25.08.23 12:05, Vik Fearing wrote:

I am replying to this email, but my comments are based on the v2 patch.

Thank you for working on this, and I think this is a valuable 
addition. However, I have two issues with it.


1) There seems to be several spurious blank lines added that I do not 
think are warranted.


I tried to copy the aesthetics of other functions, but it seems I failed 
:) I removed a few blank lines. I hope it's fine now.


I am talking specifically about this:

@@ -505,6 +506,10 @@ xmlcomment(PG_FUNCTION_ARGS)
appendStringInfoText(, arg);
appendStringInfoString(, "-->");

+
+
+
+
PG_RETURN_XML_P(stringinfo_to_xmltype());
 #else
NO_XML_SUPPORT();


2) This patch does nothing to address the  so we 
can't claim to implement X038 without a disclaimer.  Upon further 
review, the same is true of XMLCOMMENT() so maybe that is okay for 
this patch, and a more comprehensive patch for our xml features is 
necessary.


If we decide to not address this point here, I can take a look at it and 
work in a separated patch.


I do not think this should be addressed in this patch because there are 
quite a lot of functions that need to handle this.

--
Vik Fearing

Re: initdb caching during tests

[Andres Freund]

Hi,

On 2023-08-25 09:00:24 +0200, Daniel Gustafsson wrote:
> > On 25 Aug 2023, at 07:50, Thomas Munro  wrote:
> > 
> > On Fri, Aug 25, 2023 at 10:10 AM Andres Freund  wrote:
> >> Let's see what the buildfarm says - it's not inconceivable that it'll show
> >> some issues.
> > 
> > Apparently Solaris doesn't like "cp -a", per animal "margay".  I think
> > "cp -RPp" should be enough everywhere?

Thanks for noticing the issue and submitting the patch.


> Agreed, AFAICT that should work equally well on all supported platforms.

Also agreed. Unsurprisingly, CI didn't find anything on the tested platforms.

Pushed.

Greetings,

Andres Freund

Re: Let's make PostgreSQL multi-threaded

[Stephen Frost]

Greetings,

* David Geier (geidav...@gmail.com) wrote:
> On 8/11/23 14:05, Merlin Moncure wrote:
> > Hm, noted this upthread, but asking again, does this
> > help/benefit interactions with the operating system make oom kill
> > situations less likely?   These things are the bane of my existence, and
> > I'm having a hard time finding a solution that prevents them other than
> > running pgbouncer and lowering max_connections, which adds complexity. 
> > I suspect I'm not the only one dealing with this.   What's really scary
> > about these situations is they come without warning.  Here's a pretty
> > typical example per sar -r.
> > 
> > The conjecture here is that lots of idle connections make the server
> > appear to have less memory available than it looks, and sudden transient
> > demands can cause it to destabilize.
> 
> It does in the sense that your server will have more memory available in
> case you have many long living connections around. Every connection has less
> kernel memory overhead if you will. Of course even then a runaway query will
> be able to invoke the OOM killer. The unfortunate thing with the OOM killer
> is that, in my experience, it often kills the checkpointer. That's because
> the checkpointer will touch all of shared buffers over time which makes it
> likely to get selected by the OOM killer. Have you tried disabling memory
> overcommit?

This is getting a bit far afield in terms of this specific thread, but
there's an ongoing effort to give PG administrators knobs to be able to
control how much actual memory is used rather than depending on the
kernel to actually tell us when we're "out" of memory.  There'll be new
patches for the September commitfest posted soon.  If you're interested
in this issue, it'd be great to get more folks involved in review and
testing.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Format list of catalog files in makefile vertically

[Andres Freund]

Hi, 

On August 25, 2023 9:12:51 AM EDT, Peter Eisentraut  
wrote:
>I propose to reformat the catalog lists in src/backend/catalog/Makefile to be 
>more vertical, one per line.  This makes it easier to keep that list in sync 
>with src/include/catalog/meson.build, and visually compare both lists.  Also, 
>it's easier to read and edit in general.
>
>In passing, I'd also copy over some relevant comments from the makefile to 
>meson.build.  For the hypothetical future when we delete the makefiles, these 
>comments seem worth keeping.  (For fun, I tested whether the comments are 
>still true, and yes, the order still matters.)

Makes sense to me.

Andres 
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Format list of catalog files in makefile vertically

[Peter Eisentraut]

I propose to reformat the catalog lists in src/backend/catalog/Makefile 
to be more vertical, one per line.  This makes it easier to keep that 
list in sync with src/include/catalog/meson.build, and visually compare 
both lists.  Also, it's easier to read and edit in general.


In passing, I'd also copy over some relevant comments from the makefile 
to meson.build.  For the hypothetical future when we delete the 
makefiles, these comments seem worth keeping.  (For fun, I tested 
whether the comments are still true, and yes, the order still matters.)From f6f85d874ae2ca37ad2bd53dd0c12b9fb10e8cbe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 25 Aug 2023 15:07:22 +0200
Subject: [PATCH] Format list of catalog files in makefile vertically

This makes it easier to compare the lists visually with the
corresponding meson lists.

In passing, copy over some relevant comments from the makefiles to
meson.build.
---
 src/backend/catalog/Makefile| 113 +---
 src/include/catalog/meson.build |   5 ++
 2 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index a60107bf94..3e9994793d 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -55,24 +55,70 @@ include $(top_srcdir)/src/backend/common.mk
 # must appear first, and pg_statistic before pg_statistic_ext_data, and
 # there are reputedly other, undocumented ordering dependencies.
 CATALOG_HEADERS := \
-   pg_proc.h pg_type.h pg_attribute.h pg_class.h \
-   pg_attrdef.h pg_constraint.h pg_inherits.h pg_index.h pg_operator.h \
-   pg_opfamily.h pg_opclass.h pg_am.h pg_amop.h pg_amproc.h \
-   pg_language.h pg_largeobject_metadata.h pg_largeobject.h pg_aggregate.h 
\
-   pg_statistic.h pg_statistic_ext.h pg_statistic_ext_data.h \
-   pg_rewrite.h pg_trigger.h pg_event_trigger.h pg_description.h \
-   pg_cast.h pg_enum.h pg_namespace.h pg_conversion.h pg_depend.h \
-   pg_database.h pg_db_role_setting.h pg_tablespace.h \
-   pg_authid.h pg_auth_members.h pg_shdepend.h pg_shdescription.h \
-   pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
-   pg_ts_parser.h pg_ts_template.h pg_extension.h \
-   pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
-   pg_foreign_table.h pg_policy.h pg_replication_origin.h \
-   pg_default_acl.h pg_init_privs.h pg_seclabel.h pg_shseclabel.h \
-   pg_collation.h pg_parameter_acl.h pg_partitioned_table.h \
-   pg_range.h pg_transform.h \
-   pg_sequence.h pg_publication.h pg_publication_namespace.h \
-   pg_publication_rel.h pg_subscription.h pg_subscription_rel.h
+   pg_proc.h \
+   pg_type.h \
+   pg_attribute.h \
+   pg_class.h \
+   pg_attrdef.h \
+   pg_constraint.h \
+   pg_inherits.h \
+   pg_index.h \
+   pg_operator.h \
+   pg_opfamily.h \
+   pg_opclass.h \
+   pg_am.h \
+   pg_amop.h \
+   pg_amproc.h \
+   pg_language.h \
+   pg_largeobject_metadata.h \
+   pg_largeobject.h \
+   pg_aggregate.h \
+   pg_statistic.h \
+   pg_statistic_ext.h \
+   pg_statistic_ext_data.h \
+   pg_rewrite.h \
+   pg_trigger.h \
+   pg_event_trigger.h \
+   pg_description.h \
+   pg_cast.h \
+   pg_enum.h \
+   pg_namespace.h \
+   pg_conversion.h \
+   pg_depend.h \
+   pg_database.h \
+   pg_db_role_setting.h \
+   pg_tablespace.h \
+   pg_authid.h \
+   pg_auth_members.h \
+   pg_shdepend.h \
+   pg_shdescription.h \
+   pg_ts_config.h \
+   pg_ts_config_map.h \
+   pg_ts_dict.h \
+   pg_ts_parser.h \
+   pg_ts_template.h \
+   pg_extension.h \
+   pg_foreign_data_wrapper.h \
+   pg_foreign_server.h \
+   pg_user_mapping.h \
+   pg_foreign_table.h \
+   pg_policy.h \
+   pg_replication_origin.h \
+   pg_default_acl.h \
+   pg_init_privs.h \
+   pg_seclabel.h \
+   pg_shseclabel.h \
+   pg_collation.h \
+   pg_parameter_acl.h \
+   pg_partitioned_table.h \
+   pg_range.h \
+   pg_transform.h \
+   pg_sequence.h \
+   pg_publication.h \
+   pg_publication_namespace.h \
+   pg_publication_rel.h \
+   pg_subscription.h \
+   pg_subscription_rel.h
 
 GENERATED_HEADERS := $(CATALOG_HEADERS:%.h=%_d.h) schemapg.h system_fk_info.h
 
@@ -80,13 +126,30 @@ POSTGRES_BKI_SRCS := $(addprefix 
$(top_srcdir)/src/include/catalog/, $(CATALOG_H
 
 # The .dat files we need can just be listed alphabetically.
 POSTGRES_BKI_DATA = $(addprefix $(top_srcdir)/src/include/catalog/,\
-   pg_aggregate.dat pg_am.dat pg_amop.dat pg_amproc.dat pg_authid.dat \
-   pg_cast.dat pg_class.dat pg_collation.dat pg_conversion.dat \
-   pg_database.dat pg_language.dat \
-   pg_namespace.dat pg_opclass.dat pg_operator.dat pg_opfamily.dat \
-   pg_proc.dat pg_range.dat pg_tablespace.dat \
-   

Unlogged relation copy is not fsync'd

[Heikki Linnakangas]

I noticed another missing fsync() with unlogged tables, similar to the 
one at [1].


RelationCopyStorage does this:


/*
 * When we WAL-logged rel pages, we must nonetheless fsync them.  The
 * reason is that since we're copying outside shared buffers, a 
CHECKPOINT
 * occurring during the copy has no way to flush the previously written
 * data to disk (indeed it won't know the new rel even exists).  A crash
 * later on would replay WAL from the checkpoint, therefore it wouldn't
 * replay our earlier WAL entries. If we do not fsync those pages here,
 * they might still not be on disk when the crash occurs.
 */
if (use_wal ||  copying_initfork)
smgrimmedsync(dst, forkNum);


That 'copying_initfork' condition is wrong. The first hint of that is 
that 'use_wal' is always true for an init fork. I believe this was meant 
to check for 'relpersistence == RELPERSISTENCE_UNLOGGED'. Otherwise, 
this bad thing can happen:


1. Create an unlogged table
2. ALTER TABLE unlogged_tbl SET TABLESPACE ... -- This calls 
RelationCopyStorage

3. a checkpoint happens while the command is running
4. After the ALTER TABLE has finished, shut down postgres cleanly.
5. Lose power

When you recover, the unlogged table is not reset, because it was a 
clean postgres shutdown. But the part of the file that was copied after 
the checkpoint at step 3 was never fsync'd, so part of the file is lost. 
I was able to reproduce with a VM that I kill to simulate step 5.


This is the same scenario that the smgrimmedsync() call above protects 
from for WAL-logged relations. But we got the condition wrong: instead 
of worrying about the init fork, we need to call smgrimmedsync() for all 
*other* forks of an unlogged relation.


Fortunately the fix is trivial, see attached. I suspect we have similar 
problems in a few other places, though. end_heap_rewrite(), _bt_load(), 
and gist_indexsortbuild have a similar-looking smgrimmedsync() calls, 
with no consideration for unlogged relations at all. I haven't tested 
those, but they look wrong to me.


I'm also attaching the scripts I used to reproduce this, although they 
will require some manual fiddling to run.


[1] 
https://www.postgresql.org/message-id/d47d8122-415e-425c-d0a2-e0160829702d%40iki.fi


--
Heikki Linnakangas
Neon (https://neon.tech)diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 2add053489..ebb80fa822 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -533,7 +533,7 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
 	 * replay our earlier WAL entries. If we do not fsync those pages here,
 	 * they might still not be on disk when the crash occurs.
 	 */
-	if (use_wal || copying_initfork)
+	if (use_wal || relpersistence == RELPERSISTENCE_UNLOGGED)
 		smgrimmedsync(dst, forkNum);
 }
 


unlogged-fsynctest.sh
Description: application/shellscript


unlogged-verify.sh
Description: application/shellscript

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Daniel Gustafsson]

> On 25 Aug 2023, at 14:42, Jim Jones  wrote:

> Is there any tool like pgindent to take care of it automatically?

No, pgindent doesn't address whitespace, only indentation of non-whitespace.

--
Daniel Gustafsson

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Jim Jones]

Hi Vik

Thanks for reviewing my patch!

On 25.08.23 12:05, Vik Fearing wrote:

I am replying to this email, but my comments are based on the v2 patch.

Thank you for working on this, and I think this is a valuable 
addition. However, I have two issues with it.


1) There seems to be several spurious blank lines added that I do not 
think are warranted.


I tried to copy the aesthetics of other functions, but it seems I failed 
:) I removed a few blank lines. I hope it's fine now.


Is there any tool like pgindent to take care of it automatically?



2) This patch does nothing to address the  so we 
can't claim to implement X038 without a disclaimer.  Upon further 
review, the same is true of XMLCOMMENT() so maybe that is okay for 
this patch, and a more comprehensive patch for our xml features is 
necessary.


If we decide to not address this point here, I can take a look at it and 
work in a separated patch.


v3 attached.

Thanks

Jim


From 5a5302c8c8a16bf7cf26ade70a40f9e016d23bbf Mon Sep 17 00:00:00 2001
From: Jim Jones 
Date: Fri, 25 Aug 2023 14:05:39 +0200
Subject: [PATCH v3] Add XMLText function (SQL/XML X038)

This function implements the standard XMLTest function, which
converts text into xml text nodes. It uses the libxml2 function
xmlEncodeSpecialChars to escape predifined entites (&"<>), so
that those do not cause any conflict when concatenating the text
node output with existing xml documents.

This patch includes also documentation and regression tests.
---
 doc/src/sgml/func.sgml   | 30 +++
 src/backend/catalog/sql_features.txt |  2 +-
 src/backend/utils/adt/xml.c  | 26 
 src/include/catalog/pg_proc.dat  |  3 +++
 src/test/regress/expected/xml.out| 36 
 src/test/regress/expected/xml_1.out  | 23 ++
 src/test/regress/expected/xml_2.out  | 36 
 src/test/regress/sql/xml.sql |  7 ++
 8 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7a0d4b9134..2f01a2c25d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14058,6 +14058,36 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
 documents for processing in client applications.

 
+  
+xmltext
+
+
+ xmltext
+
+
+
+xmltext ( text ) xml
+
+
+
+ The function xmltext returns an XML value with a single
+ text node containing the input argument as its content. Predefined entities
+ like ampersand (), left and right angle brackets
+ (), and quotation marks ()
+ are escaped.
+
+
+
+ Example:
+
+
+   
+

 xmlcomment
 
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index b33065d7bf..680d541673 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -633,7 +633,7 @@ X034	XMLAgg			YES
 X035	XMLAgg: ORDER BY option			YES	
 X036	XMLComment			YES	
 X037	XMLPI			YES	
-X038	XMLText			NO	
+X038	XMLText			YES	
 X040	Basic table mapping			YES	
 X041	Basic table mapping: null absent			YES	
 X042	Basic table mapping: null as nil			YES	
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 866d0d649a..dd8b453b71 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -47,6 +47,7 @@
 
 #ifdef USE_LIBXML
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -505,6 +506,10 @@ xmlcomment(PG_FUNCTION_ARGS)
 	appendStringInfoText(, arg);
 	appendStringInfoString(, "-->");
 
+
+
+
+
 	PG_RETURN_XML_P(stringinfo_to_xmltype());
 #else
 	NO_XML_SUPPORT();
@@ -5006,3 +5011,24 @@ XmlTableDestroyOpaque(TableFuncScanState *state)
 	NO_XML_SUPPORT();
 #endif			/* not USE_LIBXML */
 }
+
+Datum
+xmltext(PG_FUNCTION_ARGS)
+{
+#ifdef USE_LIBXML
+	text	   *arg = PG_GETARG_TEXT_PP(0);
+	text 	   *result;
+	xmlChar*xmlbuf = NULL;
+
+	xmlbuf = xmlEncodeSpecialChars(NULL,xml_text2xmlChar(arg));
+
+	Assert(xmlbuf);
+
+	result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
+	xmlFree(xmlbuf);
+	PG_RETURN_XML_P(result);
+#else
+	NO_XML_SUPPORT();
+	return 0;
+#endif
+}
\ No newline at end of file
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..ff00c6365d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8789,6 +8789,9 @@
 { oid => '2922', descr => 'serialize an XML value to a character string',
   proname => 'text', prorettype => 'text', proargtypes => 'xml',
   prosrc => 'xmltotext' },
+{ oid => '3813', descr => 'generate XML text node',
+  proname => 'xmltext', proisstrict => 't', prorettype => 'xml',
+  proargtypes => 'text', prosrc => 'xmltext' },
 
 { oid => '2923', descr => 'map table contents to XML',
   proname => 'table_to_xml', procost => '100', provolatile => 's',
diff --git a/src/test/regress/expected/xml.out 

Re: list of acknowledgments for PG16

[Daniel Gustafsson]

> On 25 Aug 2023, at 14:22, Magnus Hagander  wrote:
> On Tue, Aug 22, 2023 at 4:03 PM Joe Conway  wrote:

>> I'm sure we could figure out how to just release the updated docs, but
>> with RC1 a week away, is it really worthwhile?
> 
> We've also been pretty strict to say that we don't *want* unreleased
> docs on the website for any of our stable branches before, so changing
> that would be a distinct policy change as well. And doing such an
> exception for just one commit seems like it's set up for problems --
> you'd then have to do another one as soon as an adjustment is made.
> And in the end, that would mean changing the policy to say that the
> "release branches documentation tracks branch tip instead of
> releases". Which I generally speaking don't think is a good idea,
> because then they don't match what people are running anymore. I think
> it only really makes sense for this one part of the docs -- even other
> changes to the REL16 docs should be excluded until the next release is
> (this time, RC1).
> 
> Bottom line is, definite -1 for doing a one-off change that violates
> the principle we're on.

Based on your reasoning above, I agree.

> Now, if we want a *separate* location where we continuously load
> branch tip docs that's a different thing and certainly something we
> could consider.

That could be useful, seeing changes rendered with the full website style is a
good way to ensure a doc patch didn't break something subtle.  As long as keep
them from being indexed by search engines and clearly separated from /docs/ it
should be fine.

--
Daniel Gustafsson

Re: list of acknowledgments for PG16

[Magnus Hagander]

On Tue, Aug 22, 2023 at 4:03 PM Joe Conway  wrote:
>
> On 8/22/23 09:44, Tom Lane wrote:
> > Alvaro Herrera  writes:
> >> On 2023-Aug-22, Peter Eisentraut wrote:
> >>> The list of acknowledgments for the PG16 release notes has been committed.
> >>> It should show up here sometime: 
> >>> .
> >
> >> Hmm, I think these docs would only regenerate during the RC1 release, so
> >> it'll be a couple of weeks, unless we manually poke the doc builder.
> >
> > Yeah.  I could produce a new set of tarballs from the v16 branch tip,
> > but I don't know the process (nor have the admin permissions) to
> > extract the HTML docs and put them on the website.
>
>
> These days the docs update is part of a scripted process for doing an
> entire release.
>
> I'm sure we could figure out how to just release the updated docs, but
> with RC1 a week away, is it really worthwhile?

We've also been pretty strict to say that we don't *want* unreleased
docs on the website for any of our stable branches before, so changing
that would be a distinct policy change as well. And doing such an
exception for just one commit seems like it's set up for problems --
you'd then have to do another one as soon as an adjustment is made.
And in the end, that would mean changing the policy to say that the
"release branches documentation tracks branch tip instead of
releases". Which I generally speaking don't think is a good idea,
because then they don't match what people are running anymore. I think
it only really makes sense for this one part of the docs -- even other
changes to the REL16 docs should be excluded until the next release is
(this time, RC1).

Bottom line is, definite -1 for doing a one-off change that violates
the principle we're on.

Now, if we want a *separate* location where we continuously load
branch tip docs that's a different thing and certainly something we
could consider.

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

Re: persist logical slots to disk during shutdown checkpoint

[vignesh C]

On Sat, 19 Aug 2023 at 11:53, Amit Kapila  wrote:
>
> It's entirely possible for a logical slot to have a confirmed_flush
> LSN higher than the last value saved on disk while not being marked as
> dirty.  It's currently not a problem to lose that value during a clean
> shutdown / restart cycle but to support the upgrade of logical slots
> [1] (see latest patch at [2]), we seem to rely on that value being
> properly persisted to disk. During the upgrade, we need to verify that
> all the data prior to shudown_checkpoint for the logical slots has
> been consumed, otherwise, the downstream may miss some data. Now, to
> ensure the same, we are planning to compare the confirm_flush LSN
> location with the latest shudown_checkpoint location which means that
> the confirm_flush LSN should be updated after restart.
>
> I think this is inefficient even without an upgrade because, after the
> restart, this may lead to decoding some data again. Say, we process
> some transactions for which we didn't send anything downstream (the
> changes got filtered) but the confirm_flush LSN is updated due to
> keepalives. As we don't flush the latest value of confirm_flush LSN,
> it may lead to processing the same changes again.

I was able to test and verify that we were not processing the same
changes again.
Note: The 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
has logs to print if a decode transaction is skipped and also a log to
mention if any operation is filtered.
The test.sh script has the steps for a) setting up logical replication
for a table b) perform insert on table that need to be published (this
will be replicated to the subscriber) c) perform insert on a table
that will not be published (this insert will be filtered, it will not
be replicated) d) sleep for 5 seconds e) stop the server f) start the
server
I used the following steps, do the following in HEAD:
a) Apply 0001-Add-logs-to-skip-transaction-filter-insert-operation.patch
patch in Head and build the binaries b) execute test.sh c) view N1.log
file to see that the insert operations were filtered again by seeing
the following logs:
LOG:  Filter insert for table tbl2
...
===restart===
...
LOG:  Skipping transaction 0/156AD10 as start decode at is greater 0/156AE40
...
LOG:  Filter insert for table tbl2

We can see that the insert operations on tbl2 which was filtered
before server was stopped is again filtered after restart too in HEAD.

Lets see that the same changes were not processed again with patch:
a) Apply v4-0001-Persist-to-disk-logical-slots-during-a-shutdown-c.patch
from [1] also apply
0001-Add-logs-to-skip-transaction-filter-insert-operation.patch patch
and build the binaries b) execute test.sh c) view N1.log file to see
that the insert operations were skipped after restart of server by
seeing the following logs:
LOG:  Filter insert for table tbl2
...
===restart===
...
Skipping transaction 0/156AD10 as start decode at is greater 0/156AFB0
...
Skipping transaction 0/156AE80 as start decode at is greater 0/156AFB0

We can see that the insert operations on tbl2 are not processed again
after restart with the patch.

[1] - 
https://www.postgresql.org/message-id/CALDaNm0VrAt24e2FxbOX6eJQ-G_tZ0gVpsFBjzQM99NxG0hZfg%40mail.gmail.com

Regards,
Vignesh
#!/bin/bash

port_publisher=5431
port_subscriber=5432
bindir=/home/vignesh/postgres/inst/bin

echo '##'
echo '#Clean up#'
echo '##'

pg_ctl stop -D data_N2
pg_ctl stop -D data_N1

rm -r data_N1 data_N2 *log

echo ''
echo '#Set up#'
echo ''

initdb -D data_N1 -U postgres
initdb -D data_N2 -U postgres

cat << EOF >> data_N1/postgresql.conf
wal_level = logical
port = $port_publisher
wal_sender_timeout = 5s
#log_min_messages = debug3
EOF

cat << EOF >> data_N2/postgresql.conf
wal_level = logical
port = $port_subscriber
EOF

# Boot database instances
pg_ctl -D data_N1 start -w -l N1.log
pg_ctl -D data_N2 start -w -l N2.log

# Setup as publisher/subscriber
psql -U postgres -p $port_publisher -c "CREATE TABLE tbl (a int, b int);"
psql -U postgres -p $port_publisher -c "CREATE TABLE tbl2 (a int, b int);"
psql -U postgres -p $port_publisher -c "CREATE EXTENSION pg_walinspect"
psql -U postgres -p $port_publisher -c "CREATE PUBLICATION pub FOR TABLE tbl;"

psql -U postgres -p $port_subscriber -c "CREATE TABLE tbl (a int, b int);"
psql -U postgres -p $port_subscriber -c "CREATE TABLE tbl2 (a int, b int);"
psql -U postgres -p $port_subscriber -c "CREATE SUBSCRIPTION sub CONNECTION 'user=postgres dbname=postgres port=$port_publisher' PUBLICATION pub WITH (copy_data = off)"

# do INSERT on publisher
psql -U postgres -p $port_publisher -c "INSERT INTO tbl VALUES (generate_series(1, 5))"
psql -U postgres -p $port_publisher -c "INSERT INTO tbl2 VALUES (generate_series(1, 5))"

# Wait short time to make sure subscriber is caught up
sleep 5s

psql -U postgres -p $port_subscriber -c "SELECT COUNT(*) FROM tbl;"

# Stop both nodes and reboot to emulate pg_upgrade
pg_ctl stop -D data_N2

Re: Let's make PostgreSQL multi-threaded

[David Geier]

Hi,

On 8/11/23 14:05, Merlin Moncure wrote:

On Thu, Jul 27, 2023 at 8:28 AM David Geier  wrote:

Hi,

On 6/7/23 23:37, Andres Freund wrote:
> I think we're starting to hit quite a few limits related to the
process model,
> particularly on bigger machines. The overhead of cross-process
context
> switches is inherently higher than switching between threads in
the same
> process - and my suspicion is that that overhead will continue to
> increase. Once you have a significant number of connections we
end up spending
> a *lot* of time in TLB misses, and that's inherent to the
process model,
> because you can't share the TLB across processes.

Another problem I haven't seen mentioned yet is the excessive kernel
memory usage because every process has its own set of page table
entries
(PTEs). Without huge pages the amount of wasted memory can be huge if
shared buffers are big.


Hm, noted this upthread, but asking again, does this 
help/benefit interactions with the operating system make oom kill 
situations less likely?   These things are the bane of my existence, 
and I'm having a hard time finding a solution that prevents them other 
than running pgbouncer and lowering max_connections, which adds 
complexity.  I suspect I'm not the only one dealing with this.  
 What's really scary about these situations is they come without 
warning.  Here's a pretty typical example per sar -r.


The conjecture here is that lots of idle connections make the server 
appear to have less memory available than it looks, and sudden 
transient demands can cause it to destabilize.


It does in the sense that your server will have more memory available in 
case you have many long living connections around. Every connection has 
less kernel memory overhead if you will. Of course even then a runaway 
query will be able to invoke the OOM killer. The unfortunate thing with 
the OOM killer is that, in my experience, it often kills the 
checkpointer. That's because the checkpointer will touch all of shared 
buffers over time which makes it likely to get selected by the OOM 
killer. Have you tried disabling memory overcommit?


--
David Geier
(ServiceNow)

Re: cataloguing NOT NULL constraints

[Alvaro Herrera]

On 2023-Aug-25, Alvaro Herrera wrote:

> I have now pushed this again.  Hopefully it'll stick this time.

Hmm, failed under the Czech locale[1]; apparently "inh_grandchld" sorts
earlier than "inh_child1" there.  I think I'll rename inh_grandchld to
inh_child3 or something like that.

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hippopotamus=2023-08-25%2011%3A33%3A07

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: [PATCH] Add native windows on arm64 support

[Daniel Gustafsson]

> On 25 Aug 2023, at 11:40, Anthony Roberts  wrote:

> * A machine for you to directly access (via a VPN)

That could be very helpful to provide to developers who are trying to debug
very wicked bugs which requires access for analysis.  Such things are on a case
by case basis of course at your discretion, but it would most likely be very
appreciated when/if the need exists.

> * A machine we just run a worker script on that automatically picks up the 
> builds as required

This sounds like what we have for the buildfarm, where machines independently
builds the branches from the Git repo by using the buildfarm client as worker.

> * Us to do downstream CI

This sounds like the CFBot, where we (currently) use Cirrus with our own
workers for building and testing the patches which are being worked on.

http://cfbot.cputube.org/

Both the two latter options are very helpful for the community, and serve two
distincly different purposes.  Option 2 is required for a platform to be
considered supported whereas option 3 assists developers in being proactive
rather than just reactive.

--
Daniel Gustafsson

Re: RFC: Logging plan of the running query

[James Coleman]

On Thu, Aug 17, 2023 at 10:02 AM torikoshia  wrote:
>
> On 2023-06-16 01:34, James Coleman wrote:
> > Attached is v28
> > which sets ProcessLogQueryPlanInterruptActive to false in errfinish
> > when necessary. Once built with those two patches I'm simply running
> > `make check`.
>
> With v28-0001 and v28-0002 patch, I confirmed backend processes consume
> huge
> amount of memory and under some environments they were terminated by OOM
> killer.
>
> This was because memory was allocated from existing memory contexts and
> they
> were not freed after ProcessLogQueryPlanInterrupt().
> Updated the patch to use dedicated memory context for
> ProcessLogQueryPlanInterrupt().
>
> Applying attached patch and v28-0002 patch, `make check` successfully
> completed after 20min and 50GB of logs on my environment.
>
> >>> On 2023-06-15 01:48, James Coleman wrote:
> >>> > The tests have been running since last night, but have been apparently
> >>> > hung now for many hours.
>
> I don't know if this has anything to do with the hung you faced, but I
> thought
> it might be possible that the large amount of memory usage resulted in
> swapping, which caused a significant delay in processing.

Ah, yes, I think that could be a possible explanation. I was delaying
on this thread because I wasn't comfortable with having caused an
issue once (even if I couldn't easily reproduce) without at least some
theory as to the cause (and a fix).

> If possible, I would be very grateful if you could try to reproduce this
> with
> the v29 patch.

I'll kick off some testing.

Thanks,
James Coleman

Re: cataloguing NOT NULL constraints

[Alvaro Herrera]

I have now pushed this again.  Hopefully it'll stick this time.

We may want to make some further tweaks to the behavior in some cases --
for example, don't disallow ALTER TABLE DROP NOT NULL when the
constraint is both inherited and has a local definition; the other
option is to mark the constraint as no longer having a local definition.
I left it the other way because that's what CHECK does; maybe we would
like to change both at once.

I ran it through CI, and the pg_upgrade test with a dump from 14's
regression test database and everything worked well, but it's been a
while since I tested the sepgsql part of it, so that might the first
thing to explode.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: [PATCH] Add native windows on arm64 support

[Anthony Roberts]

Thanks for the link - that looks basically the same as what we do for 
CMake (option 2), so should be relatively easy.


I will have a chat to relevant people about setting a machine up 
properly for it.


Thanks,
Anthony

On 25/08/2023 11:17, Michael Paquier wrote:

On Fri, Aug 25, 2023 at 10:40:39AM +0100, Anthony Roberts wrote:

Which of these are you looking for us to provide:

* A machine for you to directly access (via a VPN)
* A machine we just run a worker script on that automatically picks up the
builds as required
* Us to do downstream CI

All are possible, but preferably not option 1, as it would mean straight up
pulling out a machine from our build farm, and it has to go through all
sorts of approvals internally. If it's the only way forward I can kick it up
the chain though.

Option 2 and 3 are ones we do for various other projects (ie. 2 - CMake, 3 -
OpenSSL)

The community has its own CI facility.  Here is a link of how to set
up a machine:
https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto

And all the results are published by the maintainers of the machines
on a periodic basis where the buildfarm client code is set:
https://buildfarm.postgresql.org/cgi-bin/show_status.pl

The final result would be for you to maintain the machine so as we are
able to see if anything breaks with it.  Adding extra dependencies to
the build like OpenSSL would be nice, but based on the contents of the
patch to add this port that does not seem mandatory to me, either.

Once the machine is ready, you will need to request a buildfarm
machine name and a key to be able to begin publishing the reports of
the builds to the community buildfarm.  Once the machine is ready to
go, just let me know and I'd be OK to merge the patch (I still want to
do a final review of it in case I've missed something, but I can move
on with that before the buildfarm machine is set up).
--
Michael

Re: [PATCH] Add native windows on arm64 support

[Anthony Roberts]

Hi,

Which of these are you looking for us to provide:

* A machine for you to directly access (via a VPN)
* A machine we just run a worker script on that automatically picks up 
the builds as required

* Us to do downstream CI

All are possible, but preferably not option 1, as it would mean straight 
up pulling out a machine from our build farm, and it has to go through 
all sorts of approvals internally. If it's the only way forward I can 
kick it up the chain though.


Option 2 and 3 are ones we do for various other projects (ie. 2 - CMake, 
3 - OpenSSL)


Thanks,
Anthony

On 17/08/2023 23:28, Michael Paquier wrote:

On Thu, Aug 17, 2023 at 09:41:44AM +0100, Anthony Roberts wrote:

Just following up on this, has there been any movement?

I did see another message go into the thread here with no reply:
https://www.postgresql.org/message-id/ZKPLjLjIjwN2lxkg%40paquier.xyz

I don't have an environment to test the patch, but I don't object to
it per se.  However, I don't really want to move forward completely
blindly as well in the long-term.

As mentioned to Niyas, could it be possible to provide to the
community a buildfarm machine that would be able to test this
environment?  I am not sure what's your status on that.  Perhaps this
is already set up and you are just waiting for the patch to be merged?
--
Michael

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

[Amit Kapila]

On Fri, Aug 25, 2023 at 2:14 PM Peter Smith  wrote:
>
> Here are my review comments for patch v25-0002.
>
> In general, I feel where possible the version checking is best done in
> the "check.c" file (the filename is a hint). Most of the review
> comments below are repeating this point.
>
> ==
> Commit message.
>
> 1.
> I felt this should mention the limitation that the slot upgrade
> feature is only supported from PG17 slots upwards.
>
> ==
> doc/src/sgml/ref/pgupgrade.sgml
>
> 2.
> +
> + pg_upgrade attempts to migrate logical
> + replication slots. This helps avoid the need for manually defining the
> + same replication slots on the new publisher. Currently,
> + pg_upgrade supports migrate logical 
> replication
> + slots when the old cluster is 17.X and later.
> +
>
> Currently, pg_upgrade supports migrate
> logical replication slots when the old cluster is 17.X and later.
>
> SUGGESTION
> Migration of logical replication slots is only supported when the old
> cluster is version 17.0 or later.
>
> ==
> src/bin/pg_upgrade/check.c
>
> 3. GENERAL
>
> IMO all version checking for this feature should only be done within
> this "check.c" file as much as possible.
>
> The detailed reason for this PG17 limitation can be in the file header
> comment of "pg_upgrade.c", and then all the version checks can simply
> say something like:
> "Logical slot migration is only support for slots in PostgreSQL 17.0
> and later. See atop file pg_upgrade.c for an explanation of this
> limitation "
>

I don't think it is a good idea to move these comments atop
pg_upgrade.c as it is specific to slots. To me, the current place
proposed by the patch appears reasonable.

> ~~~
>
> 4. check_and_dump_old_cluster
>
> + /* Extract a list of logical replication slots */
> + get_logical_slot_infos();
> +
>
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the within
> get_logical_slot_infos() and put here in the caller.
>

I think we should do it where it makes more sense. As far as I can see
currently there is no such rule.

> SUGGESTION
>
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> /* Extract a list of logical replication slots */
> get_logical_slot_infos();
> }
>

I find the current place better than this suggestion.

> ~~~
>
> 5. check_new_cluster_logical_replication_slots
>
> +check_new_cluster_logical_replication_slots(void)
> +{
> + PGresult   *res;
> + PGconn*conn;
> + int nslots = count_logical_slots();
> + int max_replication_slots;
> + char*wal_level;
> +
> + /* Quick exit if there are no logical slots on the old cluster */
> + if (nslots == 0)
> + return;
>
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the count_logical_slots() and
> then this code should be written more like this:
>
> SUGGESTION (notice the quick return comment change too)
>
> int nslots = 0;
>
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> nslots = count_logical_slots();
>
> /* Quick return if there are no logical slots to be migrated. */
> if (nslots == 0)
> return;
>

+1.

> ==
> src/bin/pg_upgrade/info.c
>
> 6. GENERAL
>
> For the sake of readability it might be better to make the function
> names more explicit:
>
> get_logical_slot_infos() -> get_old_cluster_logical_slot_infos()
> count_logical_slots() -> count_old_cluster_logical_slots()
>
> ~~~
>
> 7. get_logical_slot_infos
>
> +/*
> + * get_logical_slot_infos()
> + *
> + * Higher level routine to generate LogicalSlotInfoArr for all databases.
> + *
> + * Note: This function will not do anything if the old cluster is pre-PG 17.
> + * The logical slots are not saved at shutdown, and the confirmed_flush_lsn 
> is
> + * always behind the SHUTDOWN_CHECKPOINT record. Subsequent checks done in
> + * check_for_confirmed_flush_lsn() would raise a FATAL error if such slots 
> are
> + * included.
> + */
> +void
> +get_logical_slot_infos(void)
>
> Move all this detailed explanation about the limitation to the
> file-level comment in "pg_upgrade.c". See also review comment #3.
>

-1. This is not generic enough to be moved to pg_upgrade.c.

>
> 11.
> + /*
> + * Create logical replication slots.
> + *
> + * Note: This must be done after doing the pg_resetwal command because
> + * pg_resetwal would remove required WALs.
> + */
> + if (count_logical_slots())
> + {
> + start_postmaster(_cluster, true);
> + create_logical_replication_slots();
> + stop_postmaster(false);
> + }
> +
>
> IMO it is better to do the explicit version checking here, instead of
> relying on a side-effect within the count_logical_slots() function.
>
> SUGGESTION #1
>
> /* Logical replication slot upgrade only supported for old_cluster >= PG17 */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> if (count_logical_slots())

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

[Peter Smith]

Hi Kuroda-san,

Here are my review comments for patch v25-0003.

==
src/bin/pg_upgrade/check.c

1. GENERAL

+static void check_for_confirmed_flush_lsn(void);
+static void check_for_lost_slots(void);

For more clarity, I wonder if it is better to rename some functions:

check_for_confirmed_flush_lsn() -> check_old_cluster_for_confirmed_flush_lsn()
check_for_lost_slots() -> check_old_cluster_for_lost_slots()

~~~

2.
+ /*
+ * Logical replication slots can be migrated since PG17. See comments atop
+ * get_logical_slot_infos().
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
+ {
+ check_for_lost_slots();
+
+ /*
+ * Do additional checks if a live check is not required. This requires
+ * that confirmed_flush_lsn of all the slots is the same as the latest
+ * checkpoint location, but it would be satisfied only when the server
+ * has been shut down.
+ */
+ if (!live_check)
+ check_for_confirmed_flush_lsn();
+ }
+

2a.
If my suggestions from v25-0002 [1] are adopted then this comment
needs to change to say like "See atop file pg_upgrade.c..."

~

2b.
Hmm. If my suggestions from v25-0002 [1] are adopted then the version
checking and the slot counting would *already* be in this calling
function. In that case, why can't this whole fragment be put in the
same place? E.g. IIUC there is no reason to call these at checks all
when the old_cluster slot count is already known to be 0. Similarly,
there is no reason that both these functions need to be independently
checking count_logical_slots again since we have already done that
(again, assuming my suggestions from v25-0002 [1] are adopted).

~~~

3. check_for_lost_slots

+/*
+ * Verify that all logical replication slots are usable.
+ */
+void
+check_for_lost_slots(void)

This was forward-declared to be static, but the static function
modifier is absent here.

~

4. check_for_lost_slots

+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_logical_slots() == 0)
+ return;
+

AFAICT this quick exit can be removed. See my comment #2b.

~~~

5. check_for_confirmed_flush_lsn

+check_for_confirmed_flush_lsn(void)
+{
+ int i,
+ ntups,
+ i_slotname;
+ PGresult   *res;
+ DbInfo*active_db = _cluster.dbarr.dbs[0];
+ PGconn*conn;
+
+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_logical_slots() == 0)
+ return;

AFAICT this quick exit can be removed. See my comment #2b.

==
.../t/003_logical_replication_slots.pl

6.
+# 2. Temporarily disable the subscription
+$subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub DISABLE");
 $old_publisher->stop;

In my previous 0003 review ([2] #10b) I was not questioning the need
for the $old_publisher->stop; before the pg_upgrade. I was only asking
why it was done at this location (after the DISABLE) instead of
earlier.

~~~

7.
+# Check whether changes on the new publisher get replicated to the subscriber
+$new_publisher->safe_psql('postgres',
+ "INSERT INTO tbl VALUES (generate_series(11, 20))");
+$new_publisher->wait_for_catchup('sub');
+$result = $subscriber->safe_psql('postgres', "SELECT count(*) FROM tbl");
+is($result, qq(20), 'check changes are shipped to the subscriber');

/shipped/replicated/

--
[1] My review of patch v25-0002 -
https://www.postgresql.org/message-id/CAHut%2BPtQcou3Bfm9A5SbhFuo2uKK-6u4_j_59so3skAi8Ns03A%40mail.gmail.com
[2] My review of v24-0003 -
https://www.postgresql.org/message-id/CAHut%2BPs5%3D9q1CCyrrytyv-8oUBqE6rv-%3DYFSRuuQwVf%2BsmC-Kw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: [PATCH] Add native windows on arm64 support

[Michael Paquier]

On Fri, Aug 25, 2023 at 10:40:39AM +0100, Anthony Roberts wrote:
> Which of these are you looking for us to provide:
> 
> * A machine for you to directly access (via a VPN)
> * A machine we just run a worker script on that automatically picks up the
> builds as required
> * Us to do downstream CI
> 
> All are possible, but preferably not option 1, as it would mean straight up
> pulling out a machine from our build farm, and it has to go through all
> sorts of approvals internally. If it's the only way forward I can kick it up
> the chain though.
> 
> Option 2 and 3 are ones we do for various other projects (ie. 2 - CMake, 3 -
> OpenSSL)

The community has its own CI facility.  Here is a link of how to set
up a machine:
https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto

And all the results are published by the maintainers of the machines
on a periodic basis where the buildfarm client code is set:
https://buildfarm.postgresql.org/cgi-bin/show_status.pl

The final result would be for you to maintain the machine so as we are
able to see if anything breaks with it.  Adding extra dependencies to
the build like OpenSSL would be nice, but based on the contents of the
patch to add this port that does not seem mandatory to me, either.

Once the machine is ready, you will need to request a buildfarm
machine name and a key to be able to begin publishing the reports of
the builds to the community buildfarm.  Once the machine is ready to
go, just let me know and I'd be OK to merge the patch (I still want to
do a final review of it in case I've missed something, but I can move
on with that before the buildfarm machine is set up).
--
Michael


signature.asc
Description: PGP signature

Re: subscription/015_stream sometimes breaks

[Amit Kapila]

On Thu, Aug 24, 2023 at 3:48 PM Amit Kapila  wrote:
>
> On Thu, Aug 24, 2023 at 1:20 PM Alvaro Herrera  
> wrote:
> >
> > On 2023-Aug-24, Amit Kapila wrote:
> >
> > > On Wed, Aug 23, 2023 at 1:31 PM Alvaro Herrera  
> > > wrote:
> >
> > > > Hmm, I think if worker->in_use is false, we shouldn't consult the rest
> > > > of the struct at all, so I propose to add the attached 0001 as a minimal
> > > > fix.
> > >
> > > I think that way we may need to add the check for in_use before
> > > accessing each of the LogicalRepWorker struct fields or form some rule
> > > about which fields (or places) are okay to access without checking
> > > in_use field.
> >
> > As far as I realize, we have that rule already.  It's only a few
> > relatively new places that have broken it.  I understand that the in_use
> > concept comes from the one of the same name in ReplicationSlot, except
> > that it is not at all documented in worker_internal.h.
> >
> > So I propose we do both: apply Zhijie's patch and my 0001 now; and
> > somebody gets to document the locking design for LogicalRepWorker.
> >
>
> Agreed.
>

Pushed both the patches.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Vik Fearing]

On 3/25/23 12:49, Jim Jones wrote:

Hi,

This small patch proposes the implementation of the standard SQL/XML 
function XMLText (X038). It basically converts a text parameter into an 
xml text node. It uses the libxml2 function xmlEncodeSpecialChars[1] to 
escape possible predefined entities.


This patch also contains documentation and regression tests.

Any thoughts?


I am replying to this email, but my comments are based on the v2 patch.

Thank you for working on this, and I think this is a valuable addition. 
However, I have two issues with it.


1) There seems to be several spurious blank lines added that I do not 
think are warranted.


2) This patch does nothing to address the  so we 
can't claim to implement X038 without a disclaimer.  Upon further 
review, the same is true of XMLCOMMENT() so maybe that is okay for this 
patch, and a more comprehensive patch for our xml features is necessary.

--
Vik Fearing

Re: logical_replication_mode

[Amit Kapila]

On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut  wrote:
>
> On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
> > On Friday, August 25, 2023 12:28 PM Amit Kapila  
> > wrote:
> >>
> >> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut 
> >> wrote:
> >>>
> >>> I suggest we rename this setting to something starting with debug_.
> >>> Right now, the name looks much too tempting for users to fiddle with.
> >>> I think this is similar to force_parallel_mode.
> >>>
> >>
> >> +1. How about debug_logical_replication?
> >>
> >>> Also, the descriptions in guc_tables.c could be improved.  For
> >>> example,
> >>>
> >>>   gettext_noop("Controls when to replicate or apply each change."),
> >>>
> >>> is pretty content-free and unhelpful.
> >>>
> >>
> >> The other possibility I could think of is to change short_desc as:
> >> "Allows to replicate each change for large transactions.". Do you have any
> >> better ideas?
> >
> > How about "Forces immediate streaming or serialization of changes in large
> > transactions." which is similar to the description in document.
> >
> > I agree that renaming it to debug_xx would be better and
> > here is a patch that tries to do this.
>
> Maybe debug_logical_replication is too general?  Something like
> debug_logical_replication_streaming would be more concrete.
>

Yeah, that sounds better.

>  (Or
> debug_logical_streaming.)  Is that an appropriate name for what it's doing?
>

Yes.

-- 
With Regards,
Amit Kapila.

Re: Testing autovacuum wraparound (including failsafe)

[Daniel Gustafsson]

> On 22 Aug 2023, at 07:49, Michael Paquier  wrote:
> 
> On Wed, Jul 12, 2023 at 01:47:51PM +0200, Daniel Gustafsson wrote:
>> +# bump the query timeout to avoid false negatives on slow test syetems.
>> +$ENV{PG_TEST_TIMEOUT_DEFAULT} = 600;
>> Does this actually work?  Utils.pm read the environment variable at compile
>> time in the BEGIN block so this setting won't be seen?  A quick testprogram
>> seems to confirm this but I might be missing something.
> 
> I wish that this test were cheaper, without a need to depend on
> PG_TEST_EXTRA..  Actually, note that you are forgetting to update the
> documentation of PG_TEST_EXTRA with this new value of xid_wraparound.

Agreed, it would be nice, but I don't see any way to achieve that.  I still
think the test is worthwhile to add, once the upthread mentioned issues are
resolved.

--
Daniel Gustafsson

Re: Support run-time partition pruning for hash join

[Richard Guo]

On Fri, Aug 25, 2023 at 11:03 AM David Rowley  wrote:

> On Thu, 24 Aug 2023 at 21:27, Richard Guo  wrote:
> > I think we need to solve this problem first before we can
> > make this new partition pruning mechanism some useful in practice, but
> > how?  Some thoughts currently in my mind include
> >
> > 1) we try our best to estimate the cost of this partition pruning when
> > creating hash join paths, and decide based on the cost whether to use it
> > or not.  But this does not seem to be an easy task.
>
> I think we need to consider another Hash Join path when we detect that
> the outer side of the Hash Join involves scanning a partitioned table.
>
> I'd suggest writing some cost which costs an execution of run-time
> pruning.  With LIST and RANGE you probably want something like
> cpu_operator_cost * LOG2(nparts) once for each hashed tuple to account
> for the binary search over the sorted datum array. For HASH
> partitions, something like cpu_operator_cost * npartcols once for each
> hashed tuple.
>
> You'll need to then come up with some counter costs to subtract from
> the Append/MergeAppend.  This is tricky, as discussed.  Just come up
> with something crude for now.
>
> To start with, it could just be as crude as:
>
> total_costs *= (Min(expected_outer_rows, n_append_subnodes)  /
> n_append_subnodes);
>
> i.e assume that every outer joined row will require exactly 1 new
> partition up to the total number of partitions.  That's pretty much
> worst-case, but it'll at least allow the optimisation to work for
> cases like where the hash table is expected to contain just a tiny
> number of rows (fewer than the number of partitions)
>
> To make it better, you might want to look at join selectivity
> estimation and see if you can find something there to influence
> something better.


Thank you for the suggestion.  I will take some time considering it.

When we have multiple join levels, it seems the situation becomes even
more complex.  One Append/MergeAppend node might be pruned by more than
one Hash node, and one Hash node might provide pruning for more than one
Append/MergeAppend node.  For instance, below is the plan from the test
case added in the v1 patch:

explain (analyze, costs off, summary off, timing off)
select * from tprt p1
   inner join tprt p2 on p1.col1 = p2.col1
   right join tbl1 t on p1.col1 = t.col1 and p2.col1 = t.col1;
   QUERY PLAN
-
 Hash Right Join (actual rows=2 loops=1)
   Hash Cond: ((p1.col1 = t.col1) AND (p2.col1 = t.col1))
   ->  Hash Join (actual rows=3 loops=1)
 Hash Cond: (p1.col1 = p2.col1)
 ->  Append (actual rows=3 loops=1)
   ->  Seq Scan on tprt_1 p1_1 (never executed)
   ->  Seq Scan on tprt_2 p1_2 (actual rows=3 loops=1)
   ->  Seq Scan on tprt_3 p1_3 (never executed)
   ->  Seq Scan on tprt_4 p1_4 (never executed)
   ->  Seq Scan on tprt_5 p1_5 (never executed)
   ->  Seq Scan on tprt_6 p1_6 (never executed)
 ->  Hash (actual rows=3 loops=1)
   Buckets: 1024  Batches: 1  Memory Usage: 9kB
   ->  Append (actual rows=3 loops=1)
 ->  Seq Scan on tprt_1 p2_1 (never executed)
 ->  Seq Scan on tprt_2 p2_2 (actual rows=3 loops=1)
 ->  Seq Scan on tprt_3 p2_3 (never executed)
 ->  Seq Scan on tprt_4 p2_4 (never executed)
 ->  Seq Scan on tprt_5 p2_5 (never executed)
 ->  Seq Scan on tprt_6 p2_6 (never executed)
   ->  Hash (actual rows=2 loops=1)
 Buckets: 1024  Batches: 1  Memory Usage: 9kB
 ->  Seq Scan on tbl1 t (actual rows=2 loops=1)
(23 rows)

In this plan, the Append node of 'p1' is pruned by two Hash nodes: Hash
node of 't' and Hash node of 'p2'.  Meanwhile, the Hash node of 't'
provides pruning for two Append nodes: Append node of 'p1' and Append
node of 'p2'.

In this case, meaningfully costing for the partition pruning seems even
more difficult.  Do you have any suggestion on that?


> > 2) we use some heuristics when executing hash join, such as when we
> > notice that a $threshold percentage of the partitions must be visited
> > we just abort the pruning and assume that no partitions can be pruned.
>
> You could likely code in something that checks
> bms_num_members(jpstate->part_prune_result) to see if it still remains
> below the total Append/MergeAppend subplans whenever, say whenever the
> lower 8 bits of hashtable->totalTuples are all off.  You can just give
> up doing any further pruning when all partitions are already required.


Yeah, we can do that.  While this may not help in the tests I performed
for the worst case because the table in the hash side is designed that
tuples belong to the same partition are placed together so that we need
to scan almost all its tuples before we could know that all 

Re: Synchronizing slots from primary to standby

[Alvaro Herrera]

Wait a minute ...

>From bac0fbef8b203c530e5117b0b7cfee13cfab78b9 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Sat, 22 Jul 2023 10:17:48 +
Subject: [PATCH v13 1/2] Allow logical walsenders to wait for physical
 standbys

@@ -2498,6 +2500,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, 
ReorderBufferTXN *txn,
}
else
{
+   /*
+* Before we send out the last set of changes to 
logical decoding
+* output plugin, wait for specified streaming 
replication standby
+* servers (if any) to confirm receipt of WAL upto 
commit_lsn.
+*/
+   WaitForStandbyLSN(commit_lsn);

OK, so we call this new function frequently enough -- once per
transaction, if I read this correctly?  So ...

+void
+WaitForStandbyLSN(XLogRecPtr wait_for_lsn)
+{
 ...

+   /* "*" means all logical walsenders should wait for physical standbys. 
*/
+   if (strcmp(synchronize_slot_names, "*") != 0)
+   {
+   boolshouldwait = false;
+
+   rawname = pstrdup(synchronize_slot_names);
+   SplitIdentifierString(rawname, ',', );
+
+   foreach (l, elemlist)
+   {
+   char *name = lfirst(l);
+   if (strcmp(name, NameStr(MyReplicationSlot->data.name)) 
== 0)
+   {
+   shouldwait = true;
+   break;
+   }
+   }
+
+   pfree(rawname);
+   rawname = NULL;
+   list_free(elemlist);
+   elemlist = NIL;
+
+   if (!shouldwait)
+   return;
+   }
+
+   rawname = pstrdup(standby_slot_names);
+   SplitIdentifierString(rawname, ',', );

... do we really want to be doing the GUC string parsing every time
through it?  This sounds like it could be a bottleneck, or at least slow
things down.  Maybe we should think about caching this somehow.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

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

[Peter Smith]

Here are my review comments for patch v25-0002.

In general, I feel where possible the version checking is best done in
the "check.c" file (the filename is a hint). Most of the review
comments below are repeating this point.

==
Commit message.

1.
I felt this should mention the limitation that the slot upgrade
feature is only supported from PG17 slots upwards.

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

2.
+
+ pg_upgrade attempts to migrate logical
+ replication slots. This helps avoid the need for manually defining the
+ same replication slots on the new publisher. Currently,
+ pg_upgrade supports migrate logical replication
+ slots when the old cluster is 17.X and later.
+

Currently, pg_upgrade supports migrate
logical replication slots when the old cluster is 17.X and later.

SUGGESTION
Migration of logical replication slots is only supported when the old
cluster is version 17.0 or later.

==
src/bin/pg_upgrade/check.c

3. GENERAL

IMO all version checking for this feature should only be done within
this "check.c" file as much as possible.

The detailed reason for this PG17 limitation can be in the file header
comment of "pg_upgrade.c", and then all the version checks can simply
say something like:
"Logical slot migration is only support for slots in PostgreSQL 17.0
and later. See atop file pg_upgrade.c for an explanation of this
limitation "

~~~

4. check_and_dump_old_cluster

+ /* Extract a list of logical replication slots */
+ get_logical_slot_infos();
+

IMO the version checking should only be done in the "checking"
functions, so it should be removed from the within
get_logical_slot_infos() and put here in the caller.

SUGGESTION

/* Logical slots can be migrated since PG17. */
if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
{
/* Extract a list of logical replication slots */
get_logical_slot_infos();
}

~~~

5. check_new_cluster_logical_replication_slots

+check_new_cluster_logical_replication_slots(void)
+{
+ PGresult   *res;
+ PGconn*conn;
+ int nslots = count_logical_slots();
+ int max_replication_slots;
+ char*wal_level;
+
+ /* Quick exit if there are no logical slots on the old cluster */
+ if (nslots == 0)
+ return;

IMO the version checking should only be done in the "checking"
functions, so it should be removed from the count_logical_slots() and
then this code should be written more like this:

SUGGESTION (notice the quick return comment change too)

int nslots = 0;

/* Logical slots can be migrated since PG17. */
if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
nslots = count_logical_slots();

/* Quick return if there are no logical slots to be migrated. */
if (nslots == 0)
return;

==
src/bin/pg_upgrade/info.c

6. GENERAL

For the sake of readability it might be better to make the function
names more explicit:

get_logical_slot_infos() -> get_old_cluster_logical_slot_infos()
count_logical_slots() -> count_old_cluster_logical_slots()

~~~

7. get_logical_slot_infos

+/*
+ * get_logical_slot_infos()
+ *
+ * Higher level routine to generate LogicalSlotInfoArr for all databases.
+ *
+ * Note: This function will not do anything if the old cluster is pre-PG 17.
+ * The logical slots are not saved at shutdown, and the confirmed_flush_lsn is
+ * always behind the SHUTDOWN_CHECKPOINT record. Subsequent checks done in
+ * check_for_confirmed_flush_lsn() would raise a FATAL error if such slots are
+ * included.
+ */
+void
+get_logical_slot_infos(void)

Move all this detailed explanation about the limitation to the
file-level comment in "pg_upgrade.c". See also review comment #3.

~~~

8. get_logical_slot_infos

+void
+get_logical_slot_infos(void)
+{
+ int dbnum;
+
+ /* Logical slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return;

IMO the version checking is best done in the "checking" functions. See
previous review comments about the caller of this. If you want to put
something here, then just have an Assert:

Assert(GET_MAJOR_VERSION(old_cluster.major_version) >= 1700);

~~~

9. count_logical_slots

+/*
+ * count_logical_slots()
+ *
+ * Sum up and return the number of logical replication slots for all databases.
+ */
+int
+count_logical_slots(void)
+{
+ int dbnum;
+ int slot_count = 0;
+
+ /* Quick exit if the version is prior to PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return 0;
+
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
+
+ return slot_count;
+}

IMO it is better to remove the version-checking side-effect here. Do
the version checks from the "check" functions where this is called
from. Also removing the check from here gives the ability to output
more useful messages -- e.g. review comment #11

==
src/bin/pg_upgrade/pg_upgrade.c

10. File-level comment

Add a detailed explanation about the limitation in the file-level
comment. See review comment #3 for 

Re: [PATCH] Add XMLText function (SQL/XML X038)

[Jim Jones]

On 25.03.23 13:25, I wrote:

I just realized that I forgot to add a few examples to my last message :D

postgres=# SELECT xmltext('foo ´/[({bar?})]\`');
  xmltext

 foo ´/[({bar?})]\`
(1 row)

postgres=# SELECT xmltext('foo & ');
    xmltext
---
 foo  bar
(1 row)

It seems that an encoding issue appears in the regression tests on 
Debian + Meson, 32 bit.


´ > ´
° > °

v2 attached updates the regression tests to fix it.

Jim

From 348e8952de0939c34e40283c7860aa44b66182f5 Mon Sep 17 00:00:00 2001
From: Jim Jones 
Date: Fri, 25 Aug 2023 10:14:24 +0200
Subject: [PATCH v2] Add XMLText function (SQL/XML X038)

This function implements the standard XMLTest function, which
converts text into xml text nodes. It uses the libxml2 function
xmlEncodeSpecialChars to escape predifined entites (&"<>), so
that those do not cause any conflict when concatenating the text
node output with existing xml documents.

This patch includes also documentation and regression tests.
---
 .gitignore   |  5 
 doc/src/sgml/func.sgml   | 30 +++
 src/backend/catalog/sql_features.txt |  2 +-
 src/backend/utils/adt/xml.c  | 30 +++
 src/include/catalog/pg_proc.dat  |  3 +++
 src/test/regress/expected/xml.out| 36 
 src/test/regress/expected/xml_1.out  | 23 ++
 src/test/regress/expected/xml_2.out  | 36 
 src/test/regress/sql/xml.sql |  7 ++
 9 files changed, 171 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index 4e911395fe..e724ef42a5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -43,3 +43,8 @@ lib*.pc
 /Release/
 /tmp_install/
 /portlock/
+
+
+
+.vscode/
+redeploy-testdb.sh
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7a0d4b9134..2f01a2c25d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14058,6 +14058,36 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
 documents for processing in client applications.

 
+  
+xmltext
+
+
+ xmltext
+
+
+
+xmltext ( text ) xml
+
+
+
+ The function xmltext returns an XML value with a single
+ text node containing the input argument as its content. Predefined entities
+ like ampersand (), left and right angle brackets
+ (), and quotation marks ()
+ are escaped.
+
+
+
+ Example:
+
+
+   
+

 xmlcomment
 
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index b33065d7bf..680d541673 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -633,7 +633,7 @@ X034	XMLAgg			YES
 X035	XMLAgg: ORDER BY option			YES	
 X036	XMLComment			YES	
 X037	XMLPI			YES	
-X038	XMLText			NO	
+X038	XMLText			YES	
 X040	Basic table mapping			YES	
 X041	Basic table mapping: null absent			YES	
 X042	Basic table mapping: null as nil			YES	
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 866d0d649a..592b804e36 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -47,6 +47,7 @@
 
 #ifdef USE_LIBXML
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -505,6 +506,10 @@ xmlcomment(PG_FUNCTION_ARGS)
 	appendStringInfoText(, arg);
 	appendStringInfoString(, "-->");
 
+
+
+
+
 	PG_RETURN_XML_P(stringinfo_to_xmltype());
 #else
 	NO_XML_SUPPORT();
@@ -5006,3 +5011,28 @@ XmlTableDestroyOpaque(TableFuncScanState *state)
 	NO_XML_SUPPORT();
 #endif			/* not USE_LIBXML */
 }
+
+Datum
+xmltext(PG_FUNCTION_ARGS)
+{
+#ifdef USE_LIBXML
+
+	text	   *arg = PG_GETARG_TEXT_PP(0);
+	text 	   *result;
+	xmlChar*xmlbuf = NULL;
+
+	xmlbuf = xmlEncodeSpecialChars(NULL,xml_text2xmlChar(arg));
+
+	Assert(xmlbuf);
+
+	result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
+
+	xmlFree(xmlbuf);
+
+	PG_RETURN_XML_P(result);
+
+#else
+	NO_XML_SUPPORT();
+	return 0;
+#endif
+}
\ No newline at end of file
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..ff00c6365d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8789,6 +8789,9 @@
 { oid => '2922', descr => 'serialize an XML value to a character string',
   proname => 'text', prorettype => 'text', proargtypes => 'xml',
   prosrc => 'xmltotext' },
+{ oid => '3813', descr => 'generate XML text node',
+  proname => 'xmltext', proisstrict => 't', prorettype => 'xml',
+  proargtypes => 'text', prosrc => 'xmltext' },
 
 { oid => '2923', descr => 'map table contents to XML',
   proname => 'table_to_xml', procost => '100', provolatile => 's',
diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out
index 398345ca67..13e4296bf8 100644
--- a/src/test/regress/expected/xml.out
+++ b/src/test/regress/expected/xml.out
@@ -1785,3 +1785,39 @@ SELECT * FROM XMLTABLE('.' 

Re: broken master regress tests

[Pavel Stehule]

pá 25. 8. 2023 v 8:22 odesílatel Matthias van de Meent <
boekewurm+postg...@gmail.com> napsal:

>
>
> On Fri, 25 Aug 2023, 05:54 Pavel Stehule,  wrote:
>
>> Hi
>>
>> today build is broken on my Fedora 39
>>
>> Regards
>>
>> Pavel
>>
>> make[2]: Opouští se adresář
>> „/home/pavel/src/postgresql.master/src/bin/initdb“
>> make -C pg_amcheck check
>> make[2]: Vstupuje se do adresáře
>> „/home/pavel/src/postgresql.master/src/bin/pg_amcheck“
>> [...]
>> # ERROR:  could not open file "base/16736/16781": Adresář nebo soubor
>> neexistuje
>> # '
>> # doesn't match '(?^:could not open file ".*": No such file or
>> directory)'
>>
>
> It looks like the error message matcher doesn't account for the localized
> version of "No such file or directory", might that be the issue?
>

yes

LANG=C maje check-world

fixed this issue

Regards

Pavel

>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)
>

Re: logical_replication_mode

[Peter Eisentraut]

On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:

On Friday, August 25, 2023 12:28 PM Amit Kapila  wrote:


On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut 
wrote:


I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.



+1. How about debug_logical_replication?


Also, the descriptions in guc_tables.c could be improved.  For
example,

  gettext_noop("Controls when to replicate or apply each change."),

is pretty content-free and unhelpful.



The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have any
better ideas?


How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.

I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.


Maybe debug_logical_replication is too general?  Something like 
debug_logical_replication_streaming would be more concrete.  (Or 
debug_logical_streaming.)  Is that an appropriate name for what it's doing?

Re: initdb caching during tests

[Daniel Gustafsson]

> On 25 Aug 2023, at 07:50, Thomas Munro  wrote:
> 
> On Fri, Aug 25, 2023 at 10:10 AM Andres Freund  wrote:
>> Let's see what the buildfarm says - it's not inconceivable that it'll show
>> some issues.
> 
> Apparently Solaris doesn't like "cp -a", per animal "margay".  I think
> "cp -RPp" should be enough everywhere?

Agreed, AFAICT that should work equally well on all supported platforms.

--
Daniel Gustafsson

RE: logical_replication_mode

[Zhijie Hou (Fujitsu)]

On Friday, August 25, 2023 12:28 PM Amit Kapila  wrote:
> 
> On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut 
> wrote:
> >
> > I suggest we rename this setting to something starting with debug_.
> > Right now, the name looks much too tempting for users to fiddle with.
> > I think this is similar to force_parallel_mode.
> >
> 
> +1. How about debug_logical_replication?
> 
> > Also, the descriptions in guc_tables.c could be improved.  For
> > example,
> >
> >  gettext_noop("Controls when to replicate or apply each change."),
> >
> > is pretty content-free and unhelpful.
> >
> 
> The other possibility I could think of is to change short_desc as:
> "Allows to replicate each change for large transactions.". Do you have any
> better ideas?

How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.

I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.

Best Regards,
Hou zj


0001-Rename-logical_replication_mode-to-debug_logical_rep.patch
Description:  0001-Rename-logical_replication_mode-to-debug_logical_rep.patch

Re: broken master regress tests

[Matthias van de Meent]

On Fri, 25 Aug 2023, 05:54 Pavel Stehule,  wrote:

> Hi
>
> today build is broken on my Fedora 39
>
> Regards
>
> Pavel
>
> make[2]: Opouští se adresář
> „/home/pavel/src/postgresql.master/src/bin/initdb“
> make -C pg_amcheck check
> make[2]: Vstupuje se do adresáře
> „/home/pavel/src/postgresql.master/src/bin/pg_amcheck“
> [...]
> # ERROR:  could not open file "base/16736/16781": Adresář nebo soubor
> neexistuje
> # '
> # doesn't match '(?^:could not open file ".*": No such file or
> directory)'
>

It looks like the error message matcher doesn't account for the localized
version of "No such file or directory", might that be the issue?

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Re: broken master regress tests

[Pavel Stehule]

pá 25. 8. 2023 v 8:10 odesílatel Pavel Stehule 
napsal:

> Hi
>
> pá 25. 8. 2023 v 7:38 odesílatel Thomas Munro 
> napsal:
>
>> On Fri, Aug 25, 2023 at 3:54 PM Pavel Stehule 
>> wrote:
>> > today build is broken on my Fedora 39
>>
>> Has commit 252dcb32 somehow upset some kind of bleeding edge btrfs
>> filesystem?  That's a wild guess and I can't really imagine how but
>> apparently your database files are totally messed up...
>>
>
> I use only ext4
>
> [pavel@nemesis ~]$ mount | grep home
> /dev/mapper/luks-feb21fdf-c7aa-4373-b25e-fb26d4b28216 on /home type ext4
> (rw,relatime,seclabel)
>
> but the kernel is fresh
>
> [pavel@nemesis ~]$ uname -a
> Linux nemesis 6.5.0-0.rc6.43.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Aug 14
> 17:17:41 UTC 2023 x86_64 GNU/Linux
>
>
I tested it on another comp with fresh fedora 38 installation with same
result

again I use only ext4 there



>
>
>

Re: broken master regress tests

[Pavel Stehule]

Hi

pá 25. 8. 2023 v 7:38 odesílatel Thomas Munro 
napsal:

> On Fri, Aug 25, 2023 at 3:54 PM Pavel Stehule 
> wrote:
> > today build is broken on my Fedora 39
>
> Has commit 252dcb32 somehow upset some kind of bleeding edge btrfs
> filesystem?  That's a wild guess and I can't really imagine how but
> apparently your database files are totally messed up...
>

I use only ext4

[pavel@nemesis ~]$ mount | grep home
/dev/mapper/luks-feb21fdf-c7aa-4373-b25e-fb26d4b28216 on /home type ext4
(rw,relatime,seclabel)

but the kernel is fresh

[pavel@nemesis ~]$ uname -a
Linux nemesis 6.5.0-0.rc6.43.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Aug 14
17:17:41 UTC 2023 x86_64 GNU/Linux