Re: Conflict between regression tests namespace & transactions due to recent changes

[Michael Paquier]

On Tue, May 16, 2023 at 11:02:45AM +0300, Marina Polyakova wrote:
> It confuses me a little that different methods are used for the same
> purpose. But the namespace test checks schemas. So see
> diff_abc_to_txn_table.patch which replaces abc with txn_table in the
> transaction test.

Looks OK seen from here.  Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: [DOC] Update ALTER SUBSCRIPTION documentation v3

[Robert Sjöblom]

Den ons 17 maj 2023 kl 03:18 skrev Peter Smith :
>
> + errhint("Use %s to disassociate the subscription from the slot after
> disabling the subscription.",
>
> IMO it looked strange having the word "subscription" 2x in the same sentence.
>
> Maybe you can reword the errhint like:
>
> BEFORE
> "Use %s to disassociate the subscription from the slot after disabling
> the subscription."
>
> SUGGESTION#1
> "Disable the subscription, then use %s to disassociate it from the slot."
>
> SUGGESTION#2
> "After disabling the subscription use %s to disassociate it from the slot."
>
> ~~~
>
> BTW, it is a bit difficult to follow this thread because the subject
> keeps changing.
>
> --
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

Good catch, I definitely agree. I'm sorry about changing the subject
line, I'm unaccustomed to mailing lists -- I'll leave it as it is now.

Attached is the revised version.

Best regards,
Robert Sjöblom

-- 
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för 
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är 
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag 
gått till fel adressat vänligen radera det ursprungliga meddelandet och 
underrätta avsändaren via e-post
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 8d997c983f..4be6ddb873 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -86,8 +86,9 @@ DROP SUBSCRIPTION [ IF EXISTS ] nameDROP SUBSCRIPTION command will fail.  To proceed in
-   this situation, disassociate the subscription from the replication slot by
-   executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+   this situation, first DISABLE the subscription, and then 
+   disassociate it from the replication slot by executing 
+   ALTER SUBSCRIPTION ... SET (slot_name = NONE). 
After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host.  Note that if the remote replication slot still
exists, it (and any related table synchronization slots) should then be
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e8b288d01c..c0373e5fad 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2185,7 +2185,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
 			 errmsg("could not connect to publisher when attempting to drop replication slot \"%s\": %s",
 	slotname, err),
 	/* translator: %s is an SQL ALTER command */
-			 errhint("Use %s to disassociate the subscription from the slot.",
+			 errhint("Disable the subscription, then use %s to disassociate it from the slot.",
 	 "ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
 }
 

Re: Order changes in PG16 since ICU introduction

[Jeff Davis]

On Tue, 2023-05-16 at 15:35 -0400, Jonathan S. Katz wrote:
> +  Sensitivity when determining equality, with
> +  level1 the least sensitive and
> +  identic the most sensitive. See  +  linkend="icu-collation-levels"/> for details.
> 
> This discusses equality sensitivity, but I'm not sure if I understand
> that term here. The ICU docs seem to call these "strengths"[1], maybe
> we 
> use that term to be consistent with upstream?

"Sensitivity" comes from "case sensitivity" which is more clear to me
than "strength". I added the term "strength" to correspond to the
unicode terminology, but I kept sensitivity and I tried to make it
slightly more clear.

Other than that, and I took your suggestions almost verbatim. Patch
attached. Thank you!

I also made a few other changes:

  * added paragraph transformation of '' or 'root' to the 'und'
language (root collation)
  * added paragraph that the "identic" level still performs some basic
normalization
  * added example for when full normalization matters

I should also say that I don't really understand the case when "kc" is
set to true and "ks" is level 2 or higher. If someone has an example of
where that matters, let me know.

Regards,
Jeff Davis

From 8633ec205b0b0297910cef8f931092d0c05eb3ce Mon Sep 17 00:00:00 2001
From: Jeff Davis 
Date: Thu, 27 Apr 2023 14:43:46 -0700
Subject: [PATCH v2] Doc improvements for language tags and custom ICU
 collations.

Separate the documentation for language tags themselves from the
available collation settings which can be included in a language tag.

Include tables of the available options, more details about the
effects of each option, and additional examples.

Also include an explanation of the "levels" of textual features and
how they relate to collation.

Discussion: https://postgr.es/m/25787ec7-4c04-9a8a-d241-4dc9be0b1...@postgresql.org
Reviewed-by: Jonathan S. Katz
---
 doc/src/sgml/charset.sgml | 680 +++---
 1 file changed, 559 insertions(+), 121 deletions(-)

diff --git a/doc/src/sgml/charset.sgml b/doc/src/sgml/charset.sgml
index 6dd95b8966..ea43732ec9 100644
--- a/doc/src/sgml/charset.sgml
+++ b/doc/src/sgml/charset.sgml
@@ -377,7 +377,134 @@ initdb --locale-provider=icu --icu-locale=en
 variants and customization options.

   
+  
+   ICU Locales
+   
+ICU Locale Names
+
+ The ICU format for the locale name is a Language Tag.
+
+
+CREATE COLLATION mycollation1 (PROVIDER = icu, LOCALE = 'ja-JP);
+CREATE COLLATION mycollation2 (PROVIDER = icu, LOCALE = 'fr');
+
+
+   
+   
+Locale Canonicalization and Validation
+
+ When defining a new ICU collation object or database with ICU as the
+ provider, the given locale name is transformed ("canonicalized") into a
+ language tag if not already in that form. For instance,
+
+
+CREATE COLLATION mycollation3 (PROVIDER = icu, LOCALE = 'en-US-u-kn-true');
+NOTICE:  using standard form "en-US-u-kn" for locale "en-US-u-kn-true"
+CREATE COLLATION mycollation4 (PROVIDER = icu, LOCALE = 'de_DE.utf8');
+NOTICE:  using standard form "de-DE" for locale "de_DE.utf8"
+
+
+ If you see this notice, ensure that the PROVIDER and
+ LOCALE are the expected result. For consistent results
+ when using the ICU provider, specify the canonical language tag instead of relying on the
+ transformation.
+
+
+ A locale with no language name, or the special language name
+ root, is transformed to have the language
+ und ("undefined").
+
+
+ ICU can transform most libc locale names, as well as some other formats,
+ into language tags for easier transition to ICU. If a libc locale name is
+ used in ICU, it may not have precisely the same behavior as in libc.
+
+
+ If there is a problem interpreting the locale name, or if the locale name
+ represents a language or region that ICU does not recognize, you will see
+ the following error:
+
+
+CREATE COLLATION nonsense (PROVIDER = icu, LOCALE = 'nonsense');
+ERROR:  ICU locale "nonsense" has unknown language "nonsense"
+HINT:  To disable ICU locale validation, set parameter icu_validation_level to DISABLED.
+
+
+  controls how the message is
+ reported. If set below ERROR, the collation will still
+ be created, but the behavior may not be what the user intended.
+
+   
+   
+Language Tag
+
+ A language tag, defined in BCP 47, is a standardized identifier used to
+ identify languages, regions, and other information about a locale.
+
+
+ Basic language tags are simply
+ language-region;
+ or even just language. The
+ language is a language code
+ (e.g. fr for French), and
+ region is a region code
+ (e.g. CA for Canada). Examples:
+ ja-JP, de, or
+ fr-CA.
+
+
+ Collation settings may be included in the language tag to customize
+ collation behavior. ICU allows extensive customization, 

Re: Reload configuration more frequently in apply worker.

[Amit Kapila]

On Wed, May 17, 2023 at 7:18 AM Zhijie Hou (Fujitsu)
 wrote:
>
> Currently, the main loop of apply worker looks like below[1]. Since there are
> two loops, the inner loop will keep receiving and applying message from
> publisher until no more message left. The worker only reloads the 
> configuration in
> the outer loop. This means if the publisher keeps sending messages (it could
> keep sending multiple transactions), the apply worker won't get a chance to
> update the GUCs.
>

Apart from that, I think in rare cases, it seems possible that after
the apply worker has waited for the data and just before it receives
the new replication data/message, the reload happens, then it won't
get a chance to process the reload before processing the new message.
I think such a theory can explain the rare BF failure you pointed out
later in the thread. Does that make sense?

> [1]
> for(;;) /* outer loop */
> {
> for(;;) /* inner loop */
> {
> len = walrcv_receive()
> if (len == 0)
> break;
> ...
> apply change
> }
>
> ...
> if (ConfigReloadPending)
> {
> ConfigReloadPending = false;
> ProcessConfigFile(PGC_SIGHUP);
> }
> ...
> }
>
> I think it would be better that the apply worker can reflect user's
> configuration changes sooner. To achieve this, we can add one more
> ProcessConfigFile() call in the inner loop. Attach the patch for the same. 
> What
> do you think ?
>

I think it appears to somewhat match what Tom said in the third point
in his email [1].

> BTW, I saw one BF failure[2] (it's very rare and only happened once in 4
> months) which I think is due to the low frequent reload in apply worker.
>
> The attached tap test shows how the failure happened.
>

I haven't yet tried to reproduce it but will try later sometime.
Thanks for your analysis.

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

-- 
With Regards,
Amit Kapila.

Re: cutting down the TODO list thread

[John Naylor]

On Tue, May 16, 2023 at 1:16 AM Tom Lane  wrote:
>
> Robert Haas  writes:

> >> Add support for polymorphic arguments and return types to languages
other than PL/PgSQL
> >> Add support for OUT and INOUT parameters to languages other than
PL/PgSQL

> > These actually seem like pretty interesting projects.
>
> Yeah.  I'm surprised that nobody has gotten around to scratching
> this itch yet.

Okay, keeping these.

On Tue, May 16, 2023 at 3:50 PM Alvaro Herrera 
wrote:
>
> On 2023-May-13, John Naylor wrote:

> > Consider having single-page pruning update the visibility map

> Hmm, I agree with removing the entry from the TODO list, but why is this
> something we Do Not Want?  If somebody shows up and do some analysis
> that in a certain workload it is beneficial to do this, then I don't
> think we should turn them down.

Okay, removing but not adding to Do Not Want.

On Tue, May 16, 2023 at 8:52 PM Matthias van de Meent <
boekewurm+postg...@gmail.com> wrote:
>
> On Tue, 16 May 2023 at 14:27, Robert Haas  wrote:
> >
> > On Tue, May 16, 2023 at 8:18 AM Matthias van de Meent
> >  wrote:
> > > Agreed; and that's why I'm not against removing the specific wording
> > > of the item. This may not have been clearly described in my previous
> > > mail, but I would instead like to see a TODO list item which covers
> > > the need to improve the number of cases where we provide actionable
> > > advice, and specifically those cases where there is not One Obvious
> > > Issue (OOI;s like when getting close to wraparound; or close
> > > checkpoints, or ...).
> >
> > My vote is for just removing the item, rather than putting it on the
> > not wanted list. I don't think it's useful to put things as general as
> > what you say here on the list. But putting this item in the not wanted
> > section might imply that it's not an area we're looking to improve,
> > which as you say, is false.
>
> That makes sense. Agreed.

(This was for SET PERFORMANCE_TIPS) -- removing but not adding to Do Not
Want.

I've removed all else proposed to simply remove.

Also removing "ECPG - Fix nested C comments" as done.

As for this:

On Sat, May 13, 2023 at 12:31 PM Tom Lane  wrote:

> > Improve speed of tab completion
> > -> Is this still a problem?
>
> I keep worrying that tab-complete.c will become so ungainly as to
> present a human-scale performance problem.  But there's been pretty
> much zero complaints so far.  Let's drop this one until some actual
> issue emerges.

Looking in the thread, the issue has to do with catalog queries, and in
fact I must have fat-fingered copying the entry -- it should be "Improve
speed of tab completion by using LIKE":

http://www.postgresql.org/message-id/20120821174847.gl1...@tamriel.snowman.net

I've left it alone for now just in case.

(I have yet to think about concrete revisions that seem needed, but I'll do
that separately.)

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

Re: cutting down the TODO list thread

[Julien Rouhaud]

On Tue, 16 May 2023, 02:05 Matthias van de Meent,

>
> The result I got when searching for "automatic postgresql index
> suggestions" was a combination of hypopg, pg_qualstats and some manual
> glue to get suggested indexes in the current database - but none of
> these are available in the main distribution.
>

FTR pg_qualstats has an integrated "automatic index suggestion" feature
since many years, so no glue needed.

>

Reload configuration more frequently in apply worker.

[Zhijie Hou (Fujitsu)]

Hi,

Currently, the main loop of apply worker looks like below[1]. Since there are
two loops, the inner loop will keep receiving and applying message from
publisher until no more message left. The worker only reloads the configuration 
in
the outer loop. This means if the publisher keeps sending messages (it could
keep sending multiple transactions), the apply worker won't get a chance to
update the GUCs.

[1]
for(;;) /* outer loop */
{
for(;;) /* inner loop */
{
len = walrcv_receive()
if (len == 0)
break;
...
apply change
}

...
if (ConfigReloadPending)
{
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
}
...
}

I think it would be better that the apply worker can reflect user's
configuration changes sooner. To achieve this, we can add one more
ProcessConfigFile() call in the inner loop. Attach the patch for the same. What
do you think ?

BTW, I saw one BF failure[2] (it's very rare and only happened once in 4
months) which I think is due to the low frequent reload in apply worker.

The attached tap test shows how the failure happened.

The test use streaming parallel mode and change logical_replication_mode to
immediate, we expect serialization to happen in the test. To reproduce the 
failure
easier, we need to add a sleep(1s) in the inner loop of apply worker so
that the apply worker won't be able to consume all messages quickly and will be
busy in the inner loop. Then the attached test will fail because the leader
apply didn't reload the configuration, thus serialization didn't happen.

[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-05-12%2008%3A05%3A41

Best Regards,
Hou zj


0001-Reload-configuration-more-frequently-in-apply-worker.patch
Description:  0001-Reload-configuration-more-frequently-in-apply-worker.patch

# Copyright (c) 2021-2023, PostgreSQL Global Development Group

# Test streaming of simple large transaction
use strict;
use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;

# Create publisher node
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
'logical_decoding_work_mem = 64kB');
$node_publisher->append_conf('postgresql.conf',
'logical_replication_mode = immediate');
$node_publisher->start;

# Create subscriber node
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = DEBUG1");
$node_subscriber->start;

$node_publisher->safe_psql('postgres', "CREATE TABLE test_tab_2 (a int)");

$node_subscriber->safe_psql('postgres', "CREATE TABLE test_tab_2 (a int)");

# Setup logical replication
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
$node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub FOR TABLE test_tab_2");

my $appname = 'tap_sub';


# Test using streaming mode 'on'

$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr 
application_name=$appname' PUBLICATION tap_pub WITH (streaming = parallel)"
);

# Wait for initial table sync to finish
$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
$node_publisher->wait_for_catchup($appname);

sleep 3;

$node_publisher->safe_psql('postgres',
"INSERT INTO test_tab_2 SELECT i FROM generate_series(1, 5) s(i)");

# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
'logical_replication_mode = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf', "log_min_messages = warning");
$node_subscriber->reload;

# Run a query to make sure that the reload has taken effect.
$node_subscriber->safe_psql('postgres', q{SELECT 1});

my $offset = -s $node_subscriber->logfile;

$node_publisher->safe_psql('postgres',
"INSERT INTO test_tab_2 SELECT i FROM generate_series(1, 5) s(i)");

# Ensure that the changes are serialized.
$node_subscriber->wait_for_log(
qr/LOG: ( [A-Z0-9]+:)? logical replication apply worker will serialize 
the remaining changes of remote transaction \d+ to a file/,
$offset);

$node_subscriber->stop;
$node_publisher->stop;

done_testing();

Re: benchmark results comparing versions 15.2 and 16

[Michael Paquier]

On Tue, May 16, 2023 at 06:00:00PM +0300, Alexander Lakhin wrote:
> I can confirm that the patches improve (restore) performance for my test:
> pgbench -i benchdb
> pgbench -c 10 -T 300 benchdb

Thanks for running these!

> tps (over three runs):
> HEAD (08c45ae23):
> 10238.441580, 10697.202119, 10706.764703
> 
> HEAD with the patches:
> 11134.510118, 11176.554996, 11150.338488
> 
> f193883fc~1 (240e0dbac)
> 11082.561388, 11233.604446, 11087.071768
> 
> 15.3 (8382864eb)
> 11328.699555, 11128.057883, 11057.934392

The numbers between f193883fc~1 and HEAD+patch are close to each
other.  It does not seem to make the whole difference with 15.3, but
most of it.  The difference can also be explained with some noise,
based on the number patterns of the third runs?

I have now applied the revert, ready for beta1.  Thanks for the
feedback!
--
Michael


signature.asc
Description: PGP signature

Re: [DOC] Update ALTER SUBSCRIPTION documentation v3

[Peter Smith]

+ errhint("Use %s to disassociate the subscription from the slot after
disabling the subscription.",

IMO it looked strange having the word "subscription" 2x in the same sentence.

Maybe you can reword the errhint like:

BEFORE
"Use %s to disassociate the subscription from the slot after disabling
the subscription."

SUGGESTION#1
"Disable the subscription, then use %s to disassociate it from the slot."

SUGGESTION#2
"After disabling the subscription use %s to disassociate it from the slot."

~~~

BTW, it is a bit difficult to follow this thread because the subject
keeps changing.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: running logical replication as the subscription owner

[Ajin Cherian]

On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada  wrote:
>
> On Mon, May 15, 2023 at 5:44 PM Ajin Cherian  wrote:
> >
> > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian  wrote:
> > >
> > > If nobody else is working on this, I can come up with a patch to fix this
> > >
> >
> > Attaching a patch which attempts to fix this.
> >
>
> Thank you for the patch! I think we might want to have tests for it.
>
I have updated the patch with a test case as well.

regards,
Ajin Cherian
Fujitsu Australia


v2-0001-Fix-issue-where-the-copy-command-does-not-adhere-.patch
Description: Binary data

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

[Peter Smith]

Hi Kuroda-san,

I confirmed the patch changes from v13-0001 to v14-0001 have addressed
the comments from my previous post, and the cfbot is passing OK, so I
don't have any more review comments at this time.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: smgrzeroextend clarification

[Andres Freund]

Hi,

On 2023-05-16 20:40:01 +0200, Peter Eisentraut wrote:
> On 10.05.23 20:10, Andres Freund wrote:
> > > So if you want to understand what smgrzeroextend() does, you need to
> > > mentally combine the documentation of three different functions.  Could we
> > > write documentation for each function that stands on its own?  And 
> > > document
> > > the function arguments, like what does blocknum and nblocks mean?
> > I guess it couldn't hurt. But if we go down that route, we basically need to
> > rewrite all the function headers in smgr.c, I think.
> 
> I took a stab at this, going through the function comments in smgr.c and
> md.c and try to make some things easier to follow.
> 
> - Took at out the weird leading tabs in the older comments.

I hate those. I wonder if we could devise a regex that'd remove them
tree-wide, instead of ending up doing it very slowly one by one - I've
e.g. removed them from a bunch of pgstat files. Reflowing the comments after
is probably the most painful part.

FWIW, I'd do that in a separate commit, and then add that commit to
.git-blame-ignore-revs. Otherwise blaming gets unnecessarily painful. Also
makes it easier to see the actual content changes...


> - Rephrased some comments so that smgr.c is more like an API documentation
> and md.c documents what that particular instance of that API does.
> 
> - Move the *extend and *zeroextend functions to a more sensible place among
> all the functions.  Especially since *write and *extend are very similar, it
> makes sense to have them close together.  This way, it's also easier to
> follow "this function is like that function" comments.

For me the prior location made a bit more sense - we should always extend the
file before writing or reading the relevant blocks. But I don't really care.


> - Also moved mdwriteback(), which was in some completely odd place.
> 
> - Added comments for function arguments that were previously not documented.
> 
> - Reworded the comments for *extend and *zeroextend to make more sense
> relative to each other.
> 
> - I left this for smgrzeroextend():
> 
> FIXME: why both blocknum and nblocks

I guess you're suggesting that we would do an lstat() to figure out the size
instead? Or use some cached size? That'd not be trivial to add - and it just
seems unrelated to smgzerorextend(), it's just as true for smgrextend().


> Like, what happens when blocknum is not the block right after the last
> existing block?

The same as with smgrextend().


> Do you get an implicit POSIX hole between the end of the file and the
> specified block and then an explicit hole for the next nblocks?

I don't know what you mean with an explicit hole? The whole point of
posix_fallocate() is that it actually allocates blocks - I don't really see
how such a space could be described as a hole? My understanding of the
"implicit POSIX hole" semantics is that the point precisely is that there is
*no* space allocated for the region.


> We should be clear here what the designer of this API intended.
> 
> 
> The name smgrzeroextend is not great.  The "zero" isn't what distinguishes
> it from smgrextend.

I think it's a quite distinguishing feature - you can't provide initial block
contents, which you can with smgrextend() (and we largely do). And using
fallocate() is only possible because we know that we're intending to read
zeros.


Greetings,

Andres Freund

Re: WL_SOCKET_ACCEPT fairness on Windows

[Andres Freund]

Hi,

On 2023-05-17 08:41:24 +1200, Thomas Munro wrote:
> Yeah.  No one seems to think this is worth worrying about (please
> speak up if you do).

+1 - we have much bigger fish to fry IMO.

Greetings,

Andres Freund

Re: issue with meson builds on msys2

[Andres Freund]

Hi,

On 2023-05-16 08:55:20 -0400, Andrew Dunstan wrote:
> I don't know where this all leaves us. It's still more than odd that the
> start works fine and the stop doesn't.

>From what I understand it's just a question of starting another shell, with
some redirection, after having previously started a shell, which left a
program running (thus still referencing the same console device).


> This piece of code has worked happily for years. It's only a recent
> installation or update of msys2 that's made the problem appear.

Yea, it does look like a bug somewhere. I just don't know how to make it a
small enough reproducer right now.


> I have implemented a workaround where IPC::Run is available - that means a
> little extra one-off work for people using msys2, but it's not a huge
> burden. Beyond that I don't really want to spend a lot more energy on it.

> I suppose the alternative would be to change the way the buildfarm calls
> pg_ctl stop. Do you have a concrete suggestion for that?

The easiest fix is to redirect stdin to /dev/null (or some file, if that's
easier to do portably) - that should fix the problem entirely, without needing
IPC::Run.

Greetings,

Andres Freund

Re: WL_SOCKET_ACCEPT fairness on Windows

[Jonathan S. Katz]

On 5/16/23 4:41 PM, Thomas Munro wrote:

On Wed, May 17, 2023 at 2:57 AM Jonathan S. Katz  wrote:



Given this has sat for a bit, I wanted to see if any of your thinking
has changed on whether this should be fixed for v16 or v17. I have
personally not formed an opinion yet, but per the current discussion, it
seems like this could wait?


Yeah.  No one seems to think this is worth worrying about (please
speak up if you do).  I'll go ahead and remove this from the open item
lists now, but I'll leave the patch in the CF for 17, to see if a
Windows hacker/tester thinks it's a worthwhile improvement.


[RMT hat, personal opinion]

That seems reasonable to me.

Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: Memory leak from ExecutorState context?

[Melanie Plageman]

On Sun, May 14, 2023 at 12:10:00AM +0200, Tomas Vondra wrote:
> On 5/12/23 23:36, Melanie Plageman wrote:
> > Thanks for continuing to work on this.
> > 
> > Are you planning to modify what is displayed for memory usage in
> > EXPLAIN ANALYZE?
> > 
> 
> We could do that, but we can do that separately - it's a separate and
> independent improvement, I think.
> 
> Also, do you have a proposal how to change the explain output? In
> principle we already have the number of batches, so people can calculate
> the "peak" amount of memory (assuming they realize what it means).

I don't know that we can expect people looking at the EXPLAIN output to
know how much space the different file buffers are taking up. Not to
mention that it is different for parallel hash join.

I like Jean-Guillaume's idea in his email responding to this point:

> We could add the batch memory consumption with the number of batches. Eg.:

>  Buckets: 4096 (originally 4096) 
>  Batches: 32768 (originally 8192) using 256MB
>  Memory Usage: 192kB

However, I think we can discuss this for 17.

> I think the main problem with adding this info to EXPLAIN is that I'm
> not sure it's very useful in practice. I've only really heard about this
> memory explosion issue when the query dies with OOM or takes forever,
> but EXPLAIN ANALYZE requires the query to complete.
> 
> A separate memory context (which  pg_log_backend_memory_contexts can
> dump to server log) is more valuable, I think.

Yes, I'm satisfied with scoping this to only the patch with the
dedicated memory context for now.

> > Also, since that won't help a user who OOMs, I wondered if the spillCxt
> > is helpful on its own or if we need some kind of logging message for
> > users to discover that this is what led them to running out of memory.
> > 
> 
> I think the separate memory context is definitely an improvement,
> helpful on it's own it makes it clear *what* allocated the memory. It
> requires having the memory context stats, but we may already dump them
> into the server log if malloc returns NULL. Granted, it depends on how
> the system is configured and it won't help when OOM killer hits :-(

Right. I suppose if someone had an OOM and the OOM killer ran, they may
be motivated to disable vm overcommit and then perhaps the memory
context name will show up somewhere in an error message or log?

> I wouldn't object to having some sort of log message, but when exactly
> would we emit it? Presumably after exceeding some amount of memory, but
> what would it be? It can't be too low (not to trigger it too often) or
> too high (failing to report the issue). Also, do you think it should go
> to the user or just to the server log?

I think where the log is delivered is dependent on under what conditions
we log -- if it is fairly preemptive, then doing so in the server log is
enough.

However, I think we can discuss this in the future. You are right that
the dedicated memory context by itself is an improvement.
Determining when to emit the log message seems like it will be too
difficult to accomplish in a day or so.

- Melanie

Re: WL_SOCKET_ACCEPT fairness on Windows

[Thomas Munro]

On Wed, May 17, 2023 at 2:57 AM Jonathan S. Katz  wrote:
> On 3/31/23 11:00 PM, Thomas Munro wrote:
> >>> I mention this now because I'm not sure whether to consider this an
> >>> 'open item' for 16, or merely an enhancement for 17.  I guess the
> >>> former, because someone might call that a new denial of service
> >>> vector.  On the other hand, if you fill up the listen queue for socket
> >>> 1 with enough vigour, you're also denying service to socket 1, so I
> >>> don't know if it's worth worrying about.  Opinions on that?
> >>
> >> I'm not sure either. It doesn't strike me as a particularly relevant
> >> bottleneck. And the old approach of doing more work for every single
> >> connection also made many connections worse, I think?
> >
> > Alright, let's see if anyone else thinks this is worth fixing for 16.
>
> [RMT hat]
>
> Given this has sat for a bit, I wanted to see if any of your thinking
> has changed on whether this should be fixed for v16 or v17. I have
> personally not formed an opinion yet, but per the current discussion, it
> seems like this could wait?

Yeah.  No one seems to think this is worth worrying about (please
speak up if you do).  I'll go ahead and remove this from the open item
lists now, but I'll leave the patch in the CF for 17, to see if a
Windows hacker/tester thinks it's a worthwhile improvement.

Re: Memory leak from ExecutorState context?

[Melanie Plageman]

On Tue, May 16, 2023 at 04:00:51PM +0200, Jehan-Guillaume de Rorthais wrote:

> From e5ecd466172b7bae2f1be294c1a5e70ce2b43ed8 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman 
> Date: Thu, 30 Apr 2020 07:16:28 -0700
> Subject: [PATCH v8 1/3] Describe hash join implementation
> 
> This is just a draft to spark conversation on what a good comment might
> be like in this file on how the hybrid hash join algorithm is
> implemented in Postgres. I'm pretty sure this is the accepted term for
> this algorithm https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join

I recommend changing the commit message to something like this:

Describe hash join implementation

Add details to the block comment in nodeHashjoin.c describing the
hybrid hash join algorithm at a high level.

Author: Melanie Plageman 
Author: Jehan-Guillaume de Rorthais 
Reviewed-by: Tomas Vondra 
Discussion: https://postgr.es/m/20230516160051.4267a800%40karst

> diff --git a/src/backend/executor/nodeHashjoin.c 
> b/src/backend/executor/nodeHashjoin.c
> index 0a3f32f731..93a78f6f74 100644
> --- a/src/backend/executor/nodeHashjoin.c
> +++ b/src/backend/executor/nodeHashjoin.c
> @@ -10,6 +10,47 @@
>   * IDENTIFICATION
>   * src/backend/executor/nodeHashjoin.c
>   *
> + * HASH JOIN
> + *
> + * This is based on the "hybrid hash join" algorithm described shortly in the
> + * following page, and in length in the pdf in page's notes:
> + *
> + *   https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join
> + *
> + * If the inner side tuples of a hash join do not fit in memory, the hash 
> join
> + * can be executed in multiple batches.
> + *
> + * If the statistics on the inner side relation are accurate, planner 
> chooses a
> + * multi-batch strategy and estimates the number of batches.
> + *
> + * The query executor measures the real size of the hashtable and increases 
> the
> + * number of batches if the hashtable grows too large.
> + *
> + * The number of batches is always a power of two, so an increase in the 
> number
> + * of batches doubles it.
> + *
> + * Serial hash join measures batch size lazily -- waiting until it is 
> loading a
> + * batch to determine if it will fit in memory. While inserting tuples into 
> the
> + * hashtable, serial hash join will, if that tuple were to exceed work_mem,
> + * dump out the hashtable and reassign them either to other batch files or 
> the
> + * current batch resident in the hashtable.
> + *
> + * Parallel hash join, on the other hand, completes all changes to the number
> + * of batches during the build phase. If it increases the number of batches, 
> it
> + * dumps out all the tuples from all batches and reassigns them to entirely 
> new
> + * batch files. Then it checks every batch to ensure it will fit in the space
> + * budget for the query.
> + *
> + * In both parallel and serial hash join, the executor currently makes a best
> + * effort. If a particular batch will not fit in memory, it tries doubling 
> the
> + * number of batches. If after a batch increase, there is a batch which
> + * retained all or none of its tuples, the executor disables growth in the
> + * number of batches globally. After growth is disabled, all batches that 
> would
> + * have previously triggered an increase in the number of batches instead
> + * exceed the space allowed.
> + *
> + * TODO: should we discuss that tuples can only spill forward?

I would just cut this for now since we haven't started on an agreed-upon
wording.

> From 309ad354b7a9e4dfa01b2985bd883829f5e0eba0 Mon Sep 17 00:00:00 2001
> From: Jehan-Guillaume de Rorthais 
> Date: Tue, 16 May 2023 15:42:14 +0200
> Subject: [PATCH v8 2/3] Allocate hash batches related BufFile in a dedicated
>  context

Here is a draft commit message for the second patch:

Dedicated memory context for hash join spill metadata

A hash join's hashtable may be split up into multiple batches if it
would otherwise exceed work_mem. The number of batches is doubled each
time a given batch is determined not to fit in memory. Each batch file
is allocated with a block-sized buffer for buffering tuples (parallel
hash join has additional sharedtuplestore accessor buffers).

In some cases, often with skewed data, bad stats, or very large
datasets, users can run out-of-memory while attempting to fit an
oversized batch in memory solely from the memory overhead of all the
batch files' buffers.

Batch files were allocated in the ExecutorState memory context, making
it very hard to identify when this batch explosion was the source of an
OOM. By allocating the batch files in a dedicated memory context, it
should be easier for users to identify the cause of an OOM and work to
avoid it.

I recommend editing and adding links to the various discussions on this
topic from your research.

As for the patch itself, I noticed that there are few things needing
pgindenting.

I 

Re: Order changes in PG16 since ICU introduction

[Jonathan S. Katz]

On 5/5/23 8:25 PM, Jeff Davis wrote:

On Fri, 2023-04-21 at 20:12 -0400, Robert Haas wrote:

On Fri, Apr 21, 2023 at 5:56 PM Jeff Davis  wrote:

Most of the complaints seem to be complaints about v15 as well, and
while those complaints may be a reason to not make ICU the default,
they are also an argument that we should continue to learn and try
to
fix those issues because they exist in an already-released version.
Leaving it the default for now will help us fix those issues rather
than hide them.

It's still early, so we have plenty of time to revert the initdb
default if we need to.


That's fair enough, but I really think it's important that some
energy
get invested in providing adequate documentation for this stuff. Just
patching the code is not enough.


Attached a significant documentation patch.




I tried to make it comprehensive without trying to be exhaustive, and I
separated the explanation of language tags from what collation settings
you can include in a language tag, so hopefully that's more clear.

I added quite a few examples spread throughout the various sections,
and I preserved the existing examples at the end. I also left all of
the external links at the bottom for those interested enough to go
beyond what's there.


[Personal hat, not RMT]

Thanks -- this is super helpful. A bunch of these examples I had 
previously had to figure out by randomly searching blog posts / 
trial-and-error, so I think this will help developers get started more 
quickly.


Comments (and a lot are just little nits to tighten the language)

Commit message -- typo: "documentaiton"


+ If you see such a message, ensure that the 
PROVIDER and

+ LOCALE are as you expect, and consider specifying
+ directly as the canonical language tag instead of relying on the
+ transformation.
+

I'd recommend make this more prescriptive:

"If you see this notice, ensure that the PROVIDER and 
LOCALE are the expected result. For consistent results 
when using the ICU provider, specify the canonical linkend="icu-language-tag">language tag instead of relying on the 
transformation."


+ If there is some problem interpreting the locale name, or if it 
represents
+ a language or region that ICU does not recognize, a message will 
be reported:


This is passive voice, consider:

"If there is a problem interpreting the locale name, or if the locale 
name represents a language or region that ICU does not recognize, you'll 
see the following error:"



+   
+Language Tag
+

Before jumping in, I'd recommend a quick definition of what a language 
tag is, e.g.:


"A language tag, defined in BCP 47, is a standardized identifier used to 
identify languages in computer systems" or something similar.


(I did find a database that made it simpler to search for these, which 
is one issue I've previously add, but I don't think we'd want to link to i)


+ To include this additional collation information in a language tag,
+ append -u, followed by one or more

My first question was "what's special about '-u'", so maybe we say:

"To include this additional collation information in a language tag, 
append -u, which indicates there are additional 
collation settings, followed by one or more..."


+ ICU locales are specified as a linkend="icu-language-tag">Language
+ Tag, but can also accept most libc-style locale names 
(which will

+ be transformed into language tags if possible).
+

I'd recommend removing the parantheticals:

ICU locales are specified as a BCP 47 linkend="icu-language-tag">Language
 Tag, but can also accept most libc-style locale names. If 
possible, libc-style locale names are transformed into language tags.


+  ICU Collation Levels

Nothing to add here other than to say I'm extremely appreciative of this 
section. Once upon a time I sunk a lot of time trying to figure out how 
all of these levels worked.


+  Sensitivity when determining equality, with
+  level1 the least sensitive and
+  identic the most sensitive. See  for details.

This discusses equality sensitivity, but I'm not sure if I understand 
that term here. The ICU docs seem to call these "strengths"[1], maybe we 
use that term to be consistent with upstream?


+  If set to upper, upper case sorts before lower
+  case. If set to lower, lower case sorts before
+  upper case. If set to false, it depends on the
+  locale.

Suggestion to tighten this up:

"If set to false, the sort depends on the rules of 
the locale."


+  Defaults may depend on locale. The above table is not meant to be
+  complete. See  for additinal
+  options and details.

Typo: additinal => "additional"


I didn't add additional documentation for ICU rules. There are so many
options for collations that it's hard for me to think of realistic
examples to specify the rules directly, unless someone wants to invent
a new language. Perhaps useful if working with an 

Re: Introduce "log_connection_stages" setting.

[Sergey Dudoladov]

Hello,

I have attached the fourth version of the patch.

Regards,
Sergey.
From 0303df03496ec9aafd6e69fa798177ad06a85bee Mon Sep 17 00:00:00 2001
From: Sergey Dudoladov 
Date: Tue, 8 Nov 2022 18:56:26 +0100
Subject: [PATCH] Introduce 'log_connection_messages'

This patch removes 'log_connections' and 'log_disconnections'
in favor of 'log_connection_messages', thereby introducing incompatibility

Author: Sergey Dudoladov

Reviewed-by: Justin Pryzby, Jacob Champion

Discussion:
https://www.postgresql.org/message-id/flat/CAA8Fd-qCB96uwfgMKrzfNs32mqqysi53yZFNVaRNJ6xDthZEgA%40mail.gmail.com
---
 doc/src/sgml/config.sgml  | 90 +--
 src/backend/commands/variable.c   | 81 +
 src/backend/libpq/auth.c  |  5 +-
 src/backend/postmaster/postmaster.c   |  5 +-
 src/backend/tcop/postgres.c   | 11 ++-
 src/backend/utils/init/postinit.c |  2 +-
 src/backend/utils/misc/guc_tables.c   | 30 +++
 src/backend/utils/misc/postgresql.conf.sample |  5 +-
 src/include/postgres.h|  9 ++
 src/include/postmaster/postmaster.h   |  1 -
 src/include/utils/guc_hooks.h |  2 +
 src/test/authentication/t/001_password.pl |  2 +-
 src/test/authentication/t/003_peer.pl |  2 +-
 src/test/kerberos/t/001_auth.pl   |  2 +-
 src/test/ldap/t/001_auth.pl   |  2 +-
 src/test/ldap/t/002_bindpasswd.pl |  2 +-
 src/test/recovery/t/013_crash_restart.pl  |  2 +-
 src/test/recovery/t/022_crash_temp_files.pl   |  2 +-
 src/test/recovery/t/032_relfilenode_reuse.pl  |  2 +-
 src/test/ssl/t/SSL/Server.pm  |  2 +-
 src/tools/ci/pg_ci_base.conf  |  3 +-
 21 files changed, 187 insertions(+), 75 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 2073bafa1f..42c0a1f3db 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -140,7 +140,7 @@
  An example of what this file might look like is:
 
 # This is a comment
-log_connections = yes
+log_connection_messages = all
 log_destination = 'syslog'
 search_path = '"$user", public'
 shared_buffers = 128MB
@@ -335,7 +335,7 @@ UPDATE pg_settings SET setting = reset_val WHERE name = 'configuration_parameter
passed to the postgres command via the
-c command-line parameter.  For example,
 
-postgres -c log_connections=yes -c log_destination='syslog'
+postgres -c log_connection_messages=all -c log_destination='syslog'
 
Settings provided in this way override those set via
postgresql.conf or ALTER SYSTEM,
@@ -7124,23 +7124,75 @@ local0.*/var/log/postgresql
   
  
 
- 
-  log_connections (boolean)
+ 
+  log_connection_messages (string)
   
-   log_connections configuration parameter
+   log_connection_messages configuration parameter
   
   
   

-Causes each attempted connection to the server to be logged,
-as well as successful completion of both client authentication (if
-necessary) and authorization.
+Causes the specified stages of each connection attempt to be logged.
+Example: authorized,disconnected.
+The default is the empty string, which means that nothing is logged.
 Only superusers and users with the appropriate SET
 privilege can change this parameter at session start,
 and it cannot be changed at all within a session.
-The default is off.

 
+
+ Connection messages
+ 
+  
+  
+  
+   
+Name
+Description
+   
+  
+  
+   
+received
+Logs receipt of a connection. At this point a connection has been
+received, but no further work has been done: PostgreSQL is about to start
+interacting with a client.
+   
+
+   
+authenticated
+Logs the original identity used by an authentication method
+to identify a user. In most cases, the identity string matches the
+PostgreSQL username, but some third-party authentication methods may
+alter the original user identifier before the server stores it. Failed
+authentication is always logged regardless of the value of this setting.
+
+   
+
+   
+authorized
+Logs successful completion of authorization. At this point the
+connection has been fully established.
+
+   
+
+   
+disconnected
+Logs session termination. The log output provides information
+similar to authorized, plus the duration of the session.
+
+   
+
+   
+all
+A convenience alias. If present, it must be the only value in the
+list.
+   
+
+  
+ 
+
+

 
  Some client programs, like psql, attempt
@@ -7152,26 +7204,6 @@ local0.*

Re: smgrzeroextend clarification

[Peter Eisentraut]

On 10.05.23 20:10, Andres Freund wrote:

So if you want to understand what smgrzeroextend() does, you need to
mentally combine the documentation of three different functions.  Could we
write documentation for each function that stands on its own?  And document
the function arguments, like what does blocknum and nblocks mean?

I guess it couldn't hurt. But if we go down that route, we basically need to
rewrite all the function headers in smgr.c, I think.


I took a stab at this, going through the function comments in smgr.c and 
md.c and try to make some things easier to follow.


- Took at out the weird leading tabs in the older comments.

- Rephrased some comments so that smgr.c is more like an API 
documentation and md.c documents what that particular instance of that 
API does.


- Move the *extend and *zeroextend functions to a more sensible place 
among all the functions.  Especially since *write and *extend are very 
similar, it makes sense to have them close together.  This way, it's 
also easier to follow "this function is like that function" comments.


- Also moved mdwriteback(), which was in some completely odd place.

- Added comments for function arguments that were previously not documented.

- Reworded the comments for *extend and *zeroextend to make more sense 
relative to each other.


- I left this for smgrzeroextend():

FIXME: why both blocknum and nblocks

Like, what happens when blocknum is not the block right after the last 
existing block?  Do you get an implicit POSIX hole between the end of 
the file and the specified block and then an explicit hole for the next 
nblocks?  We should be clear here what the designer of this API intended.



The name smgrzeroextend is not great.  The "zero" isn't what 
distinguishes it from smgrextend.
From fadd72afcf78a55a2cfd32217b317f17a9147962 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 16 May 2023 16:10:48 +0200
Subject: [PATCH] WIP: Improve smgr source code comments

---
 src/backend/storage/smgr/md.c   | 501 
 src/backend/storage/smgr/smgr.c | 251 
 src/include/storage/md.h|   8 +-
 src/include/storage/smgr.h  |   8 +-
 4 files changed, 382 insertions(+), 386 deletions(-)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index e982a8dd7f..4115a24b3f 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -154,7 +154,7 @@ _mdfd_open_flags(void)
 }
 
 /*
- * mdinit() -- Initialize private state for magnetic disk storage manager.
+ * mdinit() -- Initialize private state for magnetic disk storage manager.
  */
 void
 mdinit(void)
@@ -165,7 +165,7 @@ mdinit(void)
 }
 
 /*
- * mdexists() -- Does the physical file exist?
+ * mdexists() -- Does the physical file exist?
  *
  * Note: this will return true for lingering files, with pending deletions
  */
@@ -184,7 +184,7 @@ mdexists(SMgrRelation reln, ForkNumber forknum)
 }
 
 /*
- * mdcreate() -- Create a new relation on magnetic disk.
+ * mdcreate() -- Create a new relation on magnetic disk.
  *
  * If isRedo is true, it's okay for the relation to exist already.
  */
@@ -242,7 +242,7 @@ mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
 }
 
 /*
- * mdunlink() -- Unlink a relation.
+ * mdunlink() -- Unlink a relation.
  *
  * Note that we're passed a RelFileLocatorBackend --- by the time this is 
called,
  * there won't be an SMgrRelation hashtable entry anymore.
@@ -447,183 +447,7 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber 
forknum, bool isRedo)
 }
 
 /*
- * mdextend() -- Add a block to the specified relation.
- *
- * The semantics are nearly the same as mdwrite(): write at the
- * specified position.  However, this is to be used for the case of
- * extending a relation (i.e., blocknum is at or beyond the current
- * EOF).  Note that we assume writing a block beyond current EOF
- * causes intervening file space to become filled with zeroes.
- */
-void
-mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
-const void *buffer, bool skipFsync)
-{
-   off_t   seekpos;
-   int nbytes;
-   MdfdVec*v;
-
-   /* If this build supports direct I/O, the buffer must be I/O aligned. */
-   if (PG_O_DIRECT != 0 && PG_IO_ALIGN_SIZE <= BLCKSZ)
-   Assert((uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, 
buffer));
-
-   /* This assert is too expensive to have on normally ... */
-#ifdef CHECK_WRITE_VS_EXTEND
-   Assert(blocknum >= mdnblocks(reln, forknum));
-#endif
-
-   /*
-* If a relation manages to grow to 2^32-1 blocks, refuse to extend it 
any
-* more --- we mustn't create a block whose number actually is
-* InvalidBlockNumber.  (Note that this failure should be unreachable
-* because of upstream checks in bufmgr.c.)
-*/
-   if 

Re: Should CSV parsing be stricter about mid-field quotes?

[Joel Jacobson]

On Tue, May 16, 2023, at 13:43, Joel Jacobson wrote:
>If we made midfield quoting a CSV error, those users who are currently mistaken
>about their TSV/TEXT files being CSV while also having balanced quotes in their
>data, would encounter an error rather than a silent failure, which I believe
>would be an enhancement.

Furthermore, I think it could be beneficial to add a HINT message for all type
of CSV/TEXT parsing errors, since the precise ERROR messages might just cause
the user to tinker with the options until it works, instead of carefully reading
through the documentation on the various formats.

Perhaps something like this:

HINT: Are you sure the FORMAT matches your input?

Also, the COPY documentation says nothing about TSV, and I know TEXT isn't
exactly TSV, but it's at least much more TSV than CSV, so maybe we should
describe the differences, such as \N. I think the best advise to users would be
to avoid exporting to .TSV and use .CSV instead, since I've noticed e.g.
Google Sheets to replace newlines in fields with blank space when
exporting .TSV, which effectively destroys data.

The first search results for "postgresql tsv" on Google link to postgresql.org
pages, but the COPY docs are not one of them unfortunately.

The first relevant hit is this one:

"Importing a TSV File into Postgres | by Riley Wong" [1]

Sadly, this author has also misunderstood how to properly import a .TSV file,
he got it all wrong, and doesn't understand or at least doesn't mention there
are more differences than just the delimiter:

COPY listings 
FROM '/home/ec2-user/list.tsv'
DELIMITER E'\t'
CSV HEADER;

I must confess I have used PostgreSQL for over two decades without having really
understood the detailed differences between TEXT and CSV, until recently.

[1] https://medium.com/@rlwong2/importing-a-tsv-file-into-postgres-364572a004bf

Re: Order changes in PG16 since ICU introduction

[Jeff Davis]

On Tue, 2023-05-16 at 19:00 +0300, Alexander Lakhin wrote:
> I'm not sure about the proposed change in icu_from_uchar(). It seems
> that
> len_result + 1 bytes should always be enough for the result string
> terminated
> with NUL. If that's not true (we want to protect from some ICU bug
> here),
> then the change should be backpatched?

I believe it's enough and I'm not aware of any bug there so no backport
is required.

I added checks in places that were (a) checking for U_FAILURE; and (b)
expecting the result to be NUL-terminated. That's mostly callers of
uloc_getLanguage(), where I was not quite paranoid enough.

There were a couple other places though, and I went ahead and added
checks there out of paranoia, too. One was ucnv_fromUChars(), and the
other was uloc_canonicalize().

Regards,
Jeff Davis

Re: Order changes in PG16 since ICU introduction

[Alexander Lakhin]

Hi Jeff,

16.05.2023 00:03, Jeff Davis wrote:

On Sat, 2023-05-13 at 13:00 +0300, Alexander Lakhin wrote:

On the current master (after 455f948b0, and before f7faa9976, of
course)
I get an ASAN-detected failure with the following query:
CREATE COLLATION col (provider = icu, locale = '123456789012');


Thank you for the report!

ICU source specifically says:

   /**
* Useful constant for the maximum size of the language
  part of a locale ID.
* (including the terminating NULL).
* @stable ICU 2.0
*/
   #define ULOC_LANG_CAPACITY 12

So the fact that it returning success without nul-terminating the
result is an ICU bug in my opinion, and I reported it here:

https://unicode-org.atlassian.net/browse/ICU-22394

Unfortunately that means we need to be a bit more paranoid and always
check for that warning, even if we have a preallocated buffer of the
"correct" size. It also means that both U_STRING_NOT_TERMINATED_WARNING
and U_BUFFER_OVERFLOW_ERROR will be user-facing errors (potentially
scary), unless we check for those errors each time and report specific
errors for them.

Another approach is to always check the length and loop using dynamic
allocation so that we never overflow (and forget about any static
buffers). That seems like overkill given that the problem case is
obviously invalid input; I think as long as we catch it and throw an
ERROR it's fine. But I can do this if others think it's worthwhile.

Patch attached. It just checks for the U_STRING_NOT_TERMINATED_WARNING
in a few places and turns it into an ERROR. It also cleans up the loop
for uloc_getLanguageTag() to check for U_STRING_NOT_TERMINATED_WARNING
rather than (U_SUCCESS(status) && len >= buflen).


I'm not sure about the proposed change in icu_from_uchar(). It seems that
len_result + 1 bytes should always be enough for the result string terminated
with NUL. If that's not true (we want to protect from some ICU bug here),
then the change should be backpatched?

Best regards,
Alexander

Re: Move un-parenthesized syntax docs to "compatibility" for few SQL commands

[Nathan Bossart]

On Tue, May 16, 2023 at 03:28:17PM +0900, Michael Paquier wrote:
> Hmm.  This is older than the oldest version we have to support for
> pg_upgrade and co.  Would it be worth switching clusterdb to use the
> parenthesized grammar, adding on the way a test for this new grammar
> flavor without a table in the TAP tests (too costly for the main
> regression test suite)?

Since all currently-supported versions require a table name for the
parenthesized syntax, this would cause v17's clusterdb to fail on older
servers when no table name is specified.  That seems like a deal-breaker to
me.

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

Re: benchmark results comparing versions 15.2 and 16

[Alexander Lakhin]

15.05.2023 08:20, Michael Paquier wrote:

I am not going to let that hang in the air with beta1 getting released
next week, though, so attached are two patches to revert the change
(these would be combined, just posted this way for clarity).


I can confirm that the patches improve (restore) performance for my test:
pgbench -i benchdb
pgbench -c 10 -T 300 benchdb

tps (over three runs):
HEAD (08c45ae23):
10238.441580, 10697.202119, 10706.764703

HEAD with the patches:
11134.510118, 11176.554996, 11150.338488

f193883fc~1 (240e0dbac)
11082.561388, 11233.604446, 11087.071768

15.3 (8382864eb)
11328.699555, 11128.057883, 11057.934392

Best regards,
Alexander

Re: WL_SOCKET_ACCEPT fairness on Windows

[Jonathan S. Katz]

On 3/31/23 11:00 PM, Thomas Munro wrote:


I mention this now because I'm not sure whether to consider this an
'open item' for 16, or merely an enhancement for 17.  I guess the
former, because someone might call that a new denial of service
vector.  On the other hand, if you fill up the listen queue for socket
1 with enough vigour, you're also denying service to socket 1, so I
don't know if it's worth worrying about.  Opinions on that?


I'm not sure either. It doesn't strike me as a particularly relevant
bottleneck. And the old approach of doing more work for every single
connection also made many connections worse, I think?


Alright, let's see if anyone else thinks this is worth fixing for 16.


[RMT hat]

Given this has sat for a bit, I wanted to see if any of your thinking 
has changed on whether this should be fixed for v16 or v17. I have 
personally not formed an opinion yet, but per the current discussion, it 
seems like this could wait?


Thanks,

Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: Subscription statistics are not dropped at DROP SUBSCRIPTION in some cases

[Masahiko Sawada]

On Thu, May 11, 2023 at 5:12 PM Masahiko Sawada  wrote:
>
> On Wed, May 10, 2023 at 8:58 PM Melih Mutlu  wrote:
> >
> > Hi,
> >
> > Masahiko Sawada , 8 May 2023 Pzt, 10:24 tarihinde 
> > şunu yazdı:
> >>
> >> I've attached the patch. Feedback is very welcome.
> >
> >
> > Thanks for the patch, nice catch.
> > I can confirm that the issue exists on HEAD and gets resolved by this 
> > patch. Also it looks like stats are really not affected if transaction 
> > fails for some reason, as you explained.
> > IMO, the patch will be OK after commit message is added.
>
> Thank you for reviewing the patch. I'll push the patch early next
> week, barring any objections.

After thinking more about it, I realized that this is not a problem
specific to HEAD. ISTM the problem is that by commit 7b64e4b3, we drop
the stats entry of subscription that is not associated with a
replication slot for apply worker, but we missed the case where the
subscription is not associated with both replication slots for apply
and tablesync. So IIUC we should backpatch it down to 15.

Since in pg15, since we don't create the subscription stats at CREATE
SUBSCRIPTION time but do when the first error is reported, we cannot
rely on the regression test suite. Also, to check if the subscription
stats is surely removed, using pg_stat_have_stats() is clearer. So I
added a test case to TAP tests (026_stats.pl).

On Tue, May 9, 2023 at 1:51 PM Hayato Kuroda (Fujitsu)
 wrote:
>
> A comment:
>
> ```
> +   /*
> +* Tell the cumulative stats system that the subscription is getting
> +* dropped.
> +*/
> +   pgstat_drop_subscription(subid);
> ```
>
> Isn't it better to write down something you said as comment? Or is it quite 
> trivial?
>
> > There is a chance the
> > transaction dropping the subscription fails due to network error etc
> > but we don't need to worry about it as reporting the subscription drop
> > is transactional.

I'm not sure it's worth mentioning as we don't have such a comment
around other pgstat_drop_XXX functions.

Regards,

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


v2-0001-pgstat-fix-subscription-stats-entry-leak.patch
Description: Binary data

Re: pg_stat_io not tracking smgrwriteback() is confusing

[Jonathan S. Katz]

On 5/6/23 1:30 PM, Melanie Plageman wrote:


I've done that in the attached v5.


[RMT hat]

RMT nudge on this thread, as we're approaching the Beta 1 cutoff. From 
the above discussion, it sounds like it's pretty close to being ready.


Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: Assert failure of the cross-check for nullingrels

[Jonathan S. Katz]

On 5/16/23 9:49 AM, Tom Lane wrote:

"Jonathan S. Katz"  writes:

Is there a specific commit targeted for v16 that introduced this issue?
Does it only affect v16 or does it affect backbranches?


It's part of the outer-join-aware-Vars stuff, so it's my fault ...
and v16 only.


*nods* thanks. I updated the Open Items page accordingly (doing RMT 
housecleaning today in advance of Beta 1).


Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: pgbench: option delaying queries till connections establishment?

[Dave Cramer]

>
> I've recently run into something I am having difficulty understanding.
>
> I am running pgbench with the following
> pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench
> --protocol=simple
>
> Without pgbouncer I get around 5k TPS
> with pgbouncer I get around 15k TPS
>
> Looking at the code connection initiation time should not be part of the
> calculation so I' puzzled why pgbouncer is making such a dramatic
> difference ?
>
> Dave
>

Turns out that for this specific test, pg is faster with a pooler.

Dave Cramer, [May 16, 2023 at 9:49:24 AM]:

turns out having a connection pool helps. First run is without a pool,
second with


pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time,
min_exec_time, max_exec_time from pg_stat_statements where
queryid=-531095336438083412;

   mean_exec_time   |  stddev_exec_time  | calls |  total_exec_time  |
   min_exec_time
| max_exec_time

++---+---+--+---

 0.46726998 | 2.2758508661446535 |   200 | 93.453997 |
0.0466160005 | 17.434766

(1 row)


pgbench=# select pg_stat_statements_reset();

 pg_stat_statements_reset

--


(1 row)


pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time,
min_exec_time, max_exec_time from pg_stat_statements where
queryid=-531095336438083412;

   mean_exec_time|   stddev_exec_time   | calls |  total_exec_time   |
min_exec_time | max_exec_time

-+--+---++---+---

 0.066401864 | 0.021800404695481574 |   200 | 13.2803734 |
0.034006 |  0.226696
(1 row)

Re: Memory leak from ExecutorState context?

[Jehan-Guillaume de Rorthais]

Hi,

On Tue, 16 May 2023 12:01:51 +0200 Tomas Vondra 
wrote:

> On 5/16/23 00:15, Jehan-Guillaume de Rorthais wrote:
> > On Sat, 13 May 2023 23:47:53 +0200
> > Tomas Vondra  wrote:
> ...
> >> I'm not really sure about calling this "hybrid hash-join". What does it
> >> even mean? The new comments simply describe the existing batching, and
> >> how we increment number of batches, etc.  
> > 
> > Unless I'm wrong, I believed it comes from the "Hybrid hash join algorithm"
> > as described here (+ see pdf in this page ref):
> > 
> >   https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join
> > 
> > I added the ref in the v7 documentation to avoid futur confusion, is it ok?
> 
> Ah, I see. I'd just leave out the "hybrid" entirely. We've lived without
> it until now, we know what the implementation does ...

I changed the title, but kept the reference. There's still two other uses
of "hybrid hash join algorithm" in function and code comments. Keeping the ref
in this header doesn't cost much and help new comers.

> >> 0002
> >> ...
> >> The modified comment in hashjoin.h has a some alignment issues.  
> > 
> > I see no alignment issue. I suppose what bother you might be the spaces
> > before spillCxt and batchCxt to show they are childs of hashCxt? Should I
> > remove them?
> 
> It didn't occur to me this is intentional to show the contexts are
> children of hashCxt, so maybe it's not a good way to document that. I'd
> remove it, and if you think it's something worth mentioning, I'd add an
> explicit comment.

Changed.

Thanks,
>From e5ecd466172b7bae2f1be294c1a5e70ce2b43ed8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman 
Date: Thu, 30 Apr 2020 07:16:28 -0700
Subject: [PATCH v8 1/3] Describe hash join implementation

This is just a draft to spark conversation on what a good comment might
be like in this file on how the hybrid hash join algorithm is
implemented in Postgres. I'm pretty sure this is the accepted term for
this algorithm https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join
---
 src/backend/executor/nodeHashjoin.c | 41 +
 1 file changed, 41 insertions(+)

diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 0a3f32f731..93a78f6f74 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -10,6 +10,47 @@
  * IDENTIFICATION
  *	  src/backend/executor/nodeHashjoin.c
  *
+ * HASH JOIN
+ *
+ * This is based on the "hybrid hash join" algorithm described shortly in the
+ * following page, and in length in the pdf in page's notes:
+ *
+ *   https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join
+ *
+ * If the inner side tuples of a hash join do not fit in memory, the hash join
+ * can be executed in multiple batches.
+ *
+ * If the statistics on the inner side relation are accurate, planner chooses a
+ * multi-batch strategy and estimates the number of batches.
+ *
+ * The query executor measures the real size of the hashtable and increases the
+ * number of batches if the hashtable grows too large.
+ *
+ * The number of batches is always a power of two, so an increase in the number
+ * of batches doubles it.
+ *
+ * Serial hash join measures batch size lazily -- waiting until it is loading a
+ * batch to determine if it will fit in memory. While inserting tuples into the
+ * hashtable, serial hash join will, if that tuple were to exceed work_mem,
+ * dump out the hashtable and reassign them either to other batch files or the
+ * current batch resident in the hashtable.
+ *
+ * Parallel hash join, on the other hand, completes all changes to the number
+ * of batches during the build phase. If it increases the number of batches, it
+ * dumps out all the tuples from all batches and reassigns them to entirely new
+ * batch files. Then it checks every batch to ensure it will fit in the space
+ * budget for the query.
+ *
+ * In both parallel and serial hash join, the executor currently makes a best
+ * effort. If a particular batch will not fit in memory, it tries doubling the
+ * number of batches. If after a batch increase, there is a batch which
+ * retained all or none of its tuples, the executor disables growth in the
+ * number of batches globally. After growth is disabled, all batches that would
+ * have previously triggered an increase in the number of batches instead
+ * exceed the space allowed.
+ *
+ * TODO: should we discuss that tuples can only spill forward?
+ *
  * PARALLELISM
  *
  * Hash joins can participate in parallel query execution in several ways.  A
-- 
2.40.1

>From 309ad354b7a9e4dfa01b2985bd883829f5e0eba0 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais 
Date: Tue, 16 May 2023 15:42:14 +0200
Subject: [PATCH v8 2/3] Allocate hash batches related BufFile in a dedicated
 context

---
 src/backend/executor/nodeHash.c   | 43 ---
 src/backend/executor/nodeHashjoin.c   | 22 
 src/backend/utils/sort/sharedtuplestore.c |  8 +
 

Re: cutting down the TODO list thread

[Matthias van de Meent]

On Tue, 16 May 2023 at 14:27, Robert Haas  wrote:
>
> On Tue, May 16, 2023 at 8:18 AM Matthias van de Meent
>  wrote:
> > Agreed; and that's why I'm not against removing the specific wording
> > of the item. This may not have been clearly described in my previous
> > mail, but I would instead like to see a TODO list item which covers
> > the need to improve the number of cases where we provide actionable
> > advice, and specifically those cases where there is not One Obvious
> > Issue (OOI;s like when getting close to wraparound; or close
> > checkpoints, or ...).
>
> My vote is for just removing the item, rather than putting it on the
> not wanted list. I don't think it's useful to put things as general as
> what you say here on the list. But putting this item in the not wanted
> section might imply that it's not an area we're looking to improve,
> which as you say, is false.

That makes sense. Agreed.


Kind regards,

Matthias van de Meent

Re: Assert failure of the cross-check for nullingrels

[Tom Lane]

"Jonathan S. Katz"  writes:
> Is there a specific commit targeted for v16 that introduced this issue? 
> Does it only affect v16 or does it affect backbranches?

It's part of the outer-join-aware-Vars stuff, so it's my fault ...
and v16 only.

regards, tom lane

Re: Assert failure of the cross-check for nullingrels

[Jonathan S. Katz]

On 5/12/23 3:02 AM, Richard Guo wrote:


On Fri, Mar 17, 2023 at 11:05 AM Richard Guo > wrote:


Here is an updated patch with comments and test case.  I also change the
code to store 'group_clause_relids' directly in RestrictInfo.


BTW, I've added an open item for this issue.


[RMT hat]

Is there a specific commit targeted for v16 that introduced this issue? 
Does it only affect v16 or does it affect backbranches?


Thanks,

Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

Re: [DOC] Update ALTER SUBSCRIPTION documentation v3

[Robert Sjöblom]

Den tis 16 maj 2023 kl 01:44 skrev Peter Smith :
>
> On Mon, May 15, 2023 at 11:36 PM Robert Sjöblom
>  wrote:
> >
> >
> >
> > On 2023-05-05 15:17, Robert Sjöblom wrote:
> > >
> > > Hi,
> > >
> > > We have recently used the PostgreSQL documentation when setting up our
> > > logical replication. We noticed there was a step missing in the
> > > documentation on how to drop a logical replication subscription with a
> > > replication slot attached.
> >
> > Following discussions, please see revised documentation patch.
> >
>
> LGTM.
>
> BTW, in the previous thread, there was also a suggestion from Amit [1]
> to change the errhint in a similar way. There was no reply to Amit's
> idea, so it's not clear whether it's an accidental omission from your
> v2 patch or not.
>
> --
> [1] 
> https://www.postgresql.org/message-id/CAA4eK1J11phiaoCOmsjNqPZ9BOWyLXYrfgrm5vU2uCFPF2kN1Q%40mail.gmail.com
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

Accidental omission by way of mail client, I suppose -- some messages
got flagged as spam and moved to another folder. I went ahead with
Masahiko Sawada's suggestion for the error message; see revised patch.

Best regards,
Robert Sjöblom

-- 
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för 
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är 
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag 
gått till fel adressat vänligen radera det ursprungliga meddelandet och 
underrätta avsändaren via e-post
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 8d997c983f..4be6ddb873 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -86,8 +86,9 @@ DROP SUBSCRIPTION [ IF EXISTS ] nameDROP SUBSCRIPTION command will fail.  To proceed in
-   this situation, disassociate the subscription from the replication slot by
-   executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+   this situation, first DISABLE the subscription, and then 
+   disassociate it from the replication slot by executing 
+   ALTER SUBSCRIPTION ... SET (slot_name = NONE). 
After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host.  Note that if the remote replication slot still
exists, it (and any related table synchronization slots) should then be
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e8b288d01c..9ecb91ab15 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2185,7 +2185,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
 			 errmsg("could not connect to publisher when attempting to drop replication slot \"%s\": %s",
 	slotname, err),
 	/* translator: %s is an SQL ALTER command */
-			 errhint("Use %s to disassociate the subscription from the slot.",
+			 errhint("Use %s to disassociate the subscription from the slot after disabling the subscription.",
 	 "ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
 }
 

Re: issue with meson builds on msys2

[Andrew Dunstan]

On 2023-05-15 Mo 19:43, Andres Freund wrote:

Hi,

On 2023-05-15 15:30:28 -0700, Andres Freund wrote:

As soon as either the pg_ctl for the start, or the whole bash invocation, has
stdin redirected, the problem vanishes.

For a moment I thought this could be related to InheritStdHandles() - but no,
it doesn't make a difference.

There's loads of handles referencing cygwin alive in pg_ctl.

Based on difference in strace output for bash -c "pg_ctl stop" for the case
where start redirected stdin (#1) and where not (#2), it looks like some part
of msys / cygwin sees that stdin is alive when preparing to execute "pg_ctl
stop", and then runs into trouble.

The way we start the child process on windows makes the use of cmd.exe for
redirection pretty odd.


I couldn't trivially reproduce this with a much simpler case (just nohup
sleep). Perhaps it's dependent on a wrapper cmd or such.




I don't know where this all leaves us. It's still more than odd that the 
start works fine and the stop doesn't.


This piece of code has worked happily for years. It's only a recent 
installation or update of msys2 that's made the problem appear.


I have implemented a workaround where IPC::Run is available - that means 
a little extra one-off work for people using msys2, but it's not a huge 
burden. Beyond that I don't really want to spend a lot more energy on it.


I suppose the alternative would be to change the way the buildfarm calls 
pg_ctl stop. Do you have a concrete suggestion for that?



cheers


andrew

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

Re: pgbench: option delaying queries till connections establishment?

[Dave Cramer]

Dave Cramer
www.postgres.rocks


On Tue, 16 May 2023 at 07:27, Andres Freund  wrote:

> Hi,
>
> I am trying to run a few benchmarks measuring the effects of patch to
> make GetSnapshotData() faster in the face of larger numbers of
> established connections.
>
> Before the patch connection establishment often is very slow due to
> contention. The first few connections are fast, but after that it takes
> increasingly long. The first few connections constantly hold
> ProcArrayLock in shared mode, which then makes it hard for new
> connections to acquire it exclusively (I'm addressing that to a
> significant degree in the patch FWIW).
>
> But for a fair comparison of the runtime effects I'd like to only
> compare the throughput for when connections are actually usable,
> otherwise I end up benchmarking few vs many connections, which is not
> useful. And because I'd like to run the numbers for a lot of different
> numbers of connections etc, I can't just make each run several hour
> longs to make the initial minutes not matter much.
>
> Therefore I'd like to make pgbench wait till it has established all
> connections, before they run queries.
>
> Does anybody else see this as being useful?
>
> If so, should this be done unconditionally? A new option? Included in an
> existing one somehow?
>
> Greetings,

Andres Freund
>

I've recently run into something I am having difficulty understanding.

I am running pgbench with the following
pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench
--protocol=simple

Without pgbouncer I get around 5k TPS
with pgbouncer I get around 15k TPS

Looking at the code connection initiation time should not be part of the
calculation so I' puzzled why pgbouncer is making such a dramatic
difference ?

Dave

Re: 'converts internal representation to "..."' comment is confusing

[Robert Haas]

On Sun, May 14, 2023 at 9:37 PM Tom Lane  wrote:
> Steve Chavez  writes:
> > I found "..." confusing in some comments, so this patch changes it to
> > "cstring". Which seems to be the intention after all.
>
> Those comments are Berkeley-era, making them probably a decade older
> than the "cstring" pseudotype (invented in b663f3443).  Perhaps what
> you suggest is an improvement, but I'm not sure that appealing to
> original intent can make the case.

FWIW, it does seem like an improvement to me.

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

Re: cutting down the TODO list thread

[Robert Haas]

On Tue, May 16, 2023 at 8:18 AM Matthias van de Meent
 wrote:
> Agreed; and that's why I'm not against removing the specific wording
> of the item. This may not have been clearly described in my previous
> mail, but I would instead like to see a TODO list item which covers
> the need to improve the number of cases where we provide actionable
> advice, and specifically those cases where there is not One Obvious
> Issue (OOI;s like when getting close to wraparound; or close
> checkpoints, or ...).

My vote is for just removing the item, rather than putting it on the
not wanted list. I don't think it's useful to put things as general as
what you say here on the list. But putting this item in the not wanted
section might imply that it's not an area we're looking to improve,
which as you say, is false.

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

Re: running logical replication as the subscription owner

[Robert Haas]

On Tue, May 16, 2023 at 2:39 AM Amit Kapila  wrote:
> Agreed with you and Sawada-San about having a test. BTW, shall we
> slightly tweak the documentation [1]: "The subscription apply process
> will, at a session level, run with the privileges of the subscription
> owner. However, when performing an insert, update, delete, or truncate
> operation on a particular table, it will switch roles to the table
> owner and perform the operation with the table owner's privileges." to
> be bit more specific about initial sync process as well?

It doesn't seem entirely necessary to me because the initial sync is
in effect a bunch of inserts.

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

Re: cutting down the TODO list thread

[Matthias van de Meent]

On Mon, 15 May 2023 at 20:51, Robert Haas  wrote:
>
> On Mon, May 15, 2023 at 2:05 PM Matthias van de Meent
>  wrote:
> > > Add SET PERFORMANCE_TIPS option to suggest INDEX, VACUUM, VACUUM ANALYZE, 
> > > and CLUSTER
> > > -> There are external tools that help with this kind of analysis
> >
> > Althrough there are external tools which help with the analysis, the
> > silent complaint of this item seems to be "PostgreSQL doesn't provide
> > the user with actionable maintenance tasks/DDL info", and I
> > wholeheartedly agree with that.
>
> Well, if SET PERFORMANCE_TIPS is adding a new GUC, that's inherently
> something that could not happen in a contrib module, only in core. If

Why wouldn't it? pgss has (amongst others) pgss.save, other extensions
have their own GUCs, why can't this be one of them? Sure, it'd need to
be namespaced to the extension, but that's never held anyone back from
adding their own GUCs.

> it's dedicated syntax of some kind, same thing. I am not at all
> convinced that adding something spelled SET PERFORMANCE_TIPS in any
> form is a good idea,

Well, that part I agree with; the design of this
maintenance/performance helper is very different from how I would
imagine such feature to exist in or alongside PostgreSQL. I did not
make that very clear in my initial mail, sorry for that, but I
wouldn't suggest that "SET PERFORMANCE_TIPS"

> but at the very least someone would have to
> propose what the possible values of that option would be and what they
> would do in a pretty detailed way for us to decide whether we liked it
> or not. It seems to me that it would be quite difficult to get any
> kind of consensus. If you came up with several different kinds of
> performance tips and made the GUC a comma-separated list of tip types,
> I suspect that you'd get a good bit of opposition: maybe Tom would
> dislike one of the tip types for some reason, me a second, and Andres
> a third. If you remove all three of those you've gutted your
> implementation. Whee.

Yes, I know that bikeshedding and strong personal opinions are common
on this list - I know I am guilty of that as well. Yet the expectation
that people will tend to bikeshed shouldn't be a reason to move a
feature to the "Not Wanted" list. If that were the qualifier, then we
wouldn't add any new major features, because there's always going to
be some bikeshedding for major new features.

> But even if we leave the syntax aside, it is very difficult, IMHO, to
> come up with something in this area that makes sense to put in core.
> There are so many things you could warn about, and so many possible
> algorithms that you could use to emit warnings. We do have a few
> things somewhat like this in core, like the warning that checkpoints
> are happening too close together, or the hint that when you said DROP
> TABLE olders maybe you really meant DROP TABLE orders. But in those
> cases, the situation is fairly unambiguous: if your checkpoints are
> happening too close together, you should probably raise max_wal_size,
> as long as it's not going to run you out of disk space. If you
> specified a non-existent object name, you should probably correct the
> object name to something that does exist.
>
> But things like CREATE INDEX or CLUSTER are a lot trickier. I struggle
> to think of what individual PostgreSQL command would have enough
> context to know that those things are a good idea. For example,
> clustering a table on an index makes sense if (1) there are queries
> that would run faster with the clustering and (2) they are run
> frequently enough and are expensive enough that the savings would be
> material and (3) the clustering wouldn't degrade so quickly as to be
> pointless. But I don't see how it would be possible to discover this
> situation without instrumenting the whole workload, or at least having
> some trace of the workload. Even if you have the data, you probably
> need to do a bunch of number-crunching to come up with good
> recommendations, and that's expensive, and you probably have to be OK
> with a significantly higher risk of wrong answers, too, because the
> past may be different from the future, and the planner's estimates of
> what the clustering would save might be wrong.

Agreed on most parts, including the "one statement is unlikely to have
enough context". But, not all of those issues need to be tackled to
have actionable suggestions. Just a "look at 
because it is likely you left performance on the table" would be a
good start. Other examples of such suggestions would be detecting a
small fraction of hot updates vs missed-hot updates which could
produce a hint to decrease the fillfactor of a table; a high number of
table scans vs tuples returned could produce an indication about
likely missing indexes; etc. These hints wouldn't necessarily have to
be produced with per-statement hints like the TODO suggests.

> I wouldn't go so far as to say that doing anything of this sort is
> absolutely and categorically 

Re: cutting down the TODO list thread

[Robert Haas]

On Tue, May 16, 2023 at 4:50 AM Alvaro Herrera  wrote:
> Hmm, I agree with removing the entry from the TODO list, but why is this
> something we Do Not Want?  If somebody shows up and do some analysis
> that in a certain workload it is beneficial to do this, then I don't
> think we should turn them down.

Yeah, I think Do Not Want should only be to discourage people from
submitting patches for things we know were not going to do, and that
unless we think that's going to be a problem, we should just remove
items completely.

Thanks, John, for working on this, BTW.

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

Re: Should CSV parsing be stricter about mid-field quotes?

[Joel Jacobson]

On Sun, May 14, 2023, at 16:58, Andrew Dunstan wrote:
> And if people do follow the method you describe then their input with
> unescaped quotes will be rejected 999 times out of 1000. It's only cases where
> the field happens to have an even number of embedded quotes, like Joel's
> somewhat contrived example, that the input will be accepted.

I concur with Andrew that my previous example might've been somewhat
contrived, as it deliberately included two instances of the term "inches".
It's a matter of time before someone submits a review featuring an odd number of
"inches", leading to an error.

Having done some additional digging, I stumbled upon three instances [1] [2] [3]
where users have misidentified their TSV/TEXT files as CSV. In these situations,
users have been shielded from failure due to an imbalance in quotes.

However, in cases where a field is utilized to store text in which the double
quotation mark is rarely used to denote inches, but instead, for quotation
and/or HTML attributes, it's quite feasible that a large amount of
user-generated text could contain balanced quotes. Even more concerning is the
potential for cases where the vast majority of inputs may not even contain
double quotation marks at all. This would effectively render the issue 
invisible,
even upon manual data inspection.

Here's a problem scenario that I believe is plausible:

1. The user wishes to import a .TXT file into PostgreSQL.

2. The user examines the .TXT file and observes column headers separated by a
delimiter like TAB or semicolon, with subsequent rows of data also separated by
the same delimiter.

3. The user is familiar with "CSV" (412M hits on Google) but not "TSV" (48M hits
on Google), leading to a false assumption that their file is in CSV format.

4. A Google search for "import csv into postgresql" leads the user to a tutorial
titled "Import CSV File Into PostgreSQL Table". An example found therein:

COPY persons(first_name, last_name, dob, email)
FROM 'C:\sampledb\persons.csv'
DELIMITER ','
CSV HEADER;

5. The user, now confident, believes they understand how to import their "CSV"
file.

6. In contrast to the "ERROR: unterminated CSV quoted field" examples below,
this user's .TXT file contains fields with balanced midfield quote-marks:

blog_posts.txt:
id message
1 This is a bold statement

7. The user copies the COPY command from the tutorial and modifies the file path
and delimiter accordingly. The user then concludes that the code is functioning
as expected and proceeds to deploy it.

8, Weeks later, users complain about broken links in their blog posts. Upon
inspection of the blog_posts table, the user identifies an issue:

SELECT * FROM blog_posts;
id | message
+
1 | This is a bold statement
2 | Check http://example.com/?param1=Midfield quoting>this out
(2 rows)

One of the users has used balanced quotes for the href attribute, which was
imported successfully but the quotes were stripped, contrary to the intention of
preserving them.

Content of blog_posts.txt:
id message
1 This is a bold statement
2 Check http://example.com/?param1=Midfield quoting">this out

If we made midfield quoting a CSV error, those users who are currently mistaken
about their TSV/TEXT files being CSV while also having balanced quotes in their
data, would encounter an error rather than a silent failure, which I believe
would be an enhancement.

/Joel

[1] 
https://www.postgresql.org/message-id/1upfg19cru2jigbm553fugj5k6iebtd...@4ax.com
[2] 
https://stackoverflow.com/questions/44108286/unterminated-csv-quoted-field-in-postgres
[3] 
https://dba.stackexchange.com/questions/306662/unterminated-csv-quoted-field-when-to-import-csv-data-file-into-postgresql

Re: Memory leak from ExecutorState context?

[Tomas Vondra]

On 5/16/23 00:15, Jehan-Guillaume de Rorthais wrote:
> Hi,
> 
> Thanks for your review!
> 
> On Sat, 13 May 2023 23:47:53 +0200
> Tomas Vondra  wrote:
> 
>> Thanks for the patches. A couple mostly minor comments, to complement
>> Melanie's review:
>>
>> 0001
>>
>> I'm not really sure about calling this "hybrid hash-join". What does it
>> even mean? The new comments simply describe the existing batching, and
>> how we increment number of batches, etc.
> 
> Unless I'm wrong, I believed it comes from the "Hybrid hash join algorithm" as
> described here (+ see pdf in this page ref):
> 
>   https://en.wikipedia.org/wiki/Hash_join#Hybrid_hash_join
> 
> I added the ref in the v7 documentation to avoid futur confusion, is it ok?
> 

Ah, I see. I'd just leave out the "hybrid" entirely. We've lived without
it until now, we know what the implementation does ...

>> 0002
>>
>> I wouldn't call the new ExecHashJoinSaveTuple parameter spillcxt but
>> just something generic (e.g. cxt). Yes, we're passing spillCxt, but from
>> the function's POV it's just a pointer.
> 
> changed in v7.
> 
>> I also wouldn't move the ExecHashJoinSaveTuple comment inside - it just
>> needs to be reworded that we're expecting the context to be with the
>> right lifespan. The function comment is the right place to document such
>> expectations, people are less likely to read the function body.
> 
> moved and reworded in v7.
> 
>> The modified comment in hashjoin.h has a some alignment issues.
> 
> I see no alignment issue. I suppose what bother you might be the spaces
> before spillCxt and batchCxt to show they are childs of hashCxt? Should I
> remove them?
> 

It didn't occur to me this is intentional to show the contexts are
children of hashCxt, so maybe it's not a good way to document that. I'd
remove it, and if you think it's something worth mentioning, I'd add an
explicit comment.


regards

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

Re: de-catalog one error message

[Alvaro Herrera]

On 2023-May-11, Daniel Gustafsson wrote:

> > On 10 May 2023, at 19:54, Alvaro Herrera  wrote:

> > Therefore, I propose to turn these messages into errmsg_internal(), so
> > that we do not translate them.
> 
> AFAICT from following the code that seems correct, and I agree with removing
> them from translation to lessen the burden on our translators.

Thanks for looking!  Pushed now.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Re: pgsql: Clean up role created in new subscription test.

[Daniel Gustafsson]

> On 15 May 2023, at 10:59, Daniel Gustafsson  wrote:

> Looking at this I also found a bug introduced in the TAP format patch, which
> made failed single run tests report as 0ms due to the parameters being mixed 
> up
> in the report function call.  This is in 0002, which I'll apply to HEAD
> regardless of 0001 as they are unrelated.

With 0002 applied, attached is just the 0001 rebased to keep the CFBot from
being angry when applying an already applied patch.  Parked in the July CF for
now.

--
Daniel Gustafsson



v2-0001-pg_regress-Add-database-verification-test.patch
Description: Binary data

Re: cutting down the TODO list thread

[Alvaro Herrera]

On 2023-May-13, John Naylor wrote:

> 
> 2. Propose to move to the "Not Wanted list":

> Consider having single-page pruning update the visibility map
> -> Comment from Heikki in the thread:
> "I think I was worried about the possible performance impact of having to
> clear the bit in visibility map again. If you're frequently updating a
> tuple so that HOT and page pruning is helping you, setting the bit in
> visibility map seems counter-productive; it's going to be cleared soon
> again by another UPDATE. That's just a hunch, though. Maybe the overhead
> is negligible."

Hmm, I agree with removing the entry from the TODO list, but why is this
something we Do Not Want?  If somebody shows up and do some analysis
that in a certain workload it is beneficial to do this, then I don't
think we should turn them down.

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

Re: Using make_ctags leaves tags files in git

[Daniel Gustafsson]

> On 15 May 2023, at 12:33, Alvaro Herrera  wrote:
> 
> On 2023-May-14, Tom Lane wrote:
> 
>> Steve Chavez  writes:
>>> In this case I just propose adding 'tags'. I believe it's reasonable to
>>> ignore these as they're produced by make_ctags.
>> 
>> Our policy on this is that the project's .gitignore files should ignore
>> files that are produced by our standard build scripts.
> 
> But make_ctags is *our* script, so I think this rule applies to them as
> well.

It is our script, but "tags" is not an artifact created by the build scripts
since make_{c|e}tags isn't part of the build so I'm not convinced we should
bother.  If we do I'm sure there are many other scripts in src/tools which
generate files we don't track (like make_mkid, which admittedly probably hasn't
been executed by anyone in a decade or so).

--
Daniel Gustafsson

Re: Conflict between regression tests namespace & transactions due to recent changes

[Marina Polyakova]

On 2023-05-16 02:19, Michael Paquier wrote:

On Mon, May 15, 2023 at 11:23:18PM +0300, Marina Polyakova wrote:
Maybe use a separate schema for all new objects in the transaction 
test?..

See diff_set_tx_schema.patch.


Sure, you could do that to bypass the failure (without the "public"
actually?), leaving non-generic names around.  Still I'd agree with
Tom here and just rename the objects to something more in line with
the context of the test to make things a bit more greppable.  These
could be renamed as transaction_tab or transaction_view, for example.
--
Michael


It confuses me a little that different methods are used for the same 
purpose. But the namespace test checks schemas. So see 
diff_abc_to_txn_table.patch which replaces abc with txn_table in the 
transaction test.


--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 2b2cff7d9120a139532d1a6a2f2bc79e463bc4fa..21efb900ef1bc30338f1102f6a40f1d25be0db9c 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -609,10 +609,10 @@ drop function inverse(int);
 -- performed in the aborted subtransaction
 begin;
 savepoint x;
-create table abc (a int);
-insert into abc values (5);
-insert into abc values (10);
-declare foo cursor for select * from abc;
+create table txn_table (a int);
+insert into txn_table values (5);
+insert into txn_table values (10);
+declare foo cursor for select * from txn_table;
 fetch from foo;
  a 
 ---
@@ -625,11 +625,11 @@ fetch from foo;
 ERROR:  cursor "foo" does not exist
 commit;
 begin;
-create table abc (a int);
-insert into abc values (5);
-insert into abc values (10);
-insert into abc values (15);
-declare foo cursor for select * from abc;
+create table txn_table (a int);
+insert into txn_table values (5);
+insert into txn_table values (10);
+insert into txn_table values (15);
+declare foo cursor for select * from txn_table;
 fetch from foo;
  a 
 ---
@@ -698,7 +698,7 @@ COMMIT;
 DROP FUNCTION create_temp_tab();
 DROP FUNCTION invert(x float8);
 -- Tests for AND CHAIN
-CREATE TABLE abc (a int);
+CREATE TABLE txn_table (a int);
 -- set nondefault value so we have something to override below
 SET default_transaction_read_only = on;
 START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
@@ -720,8 +720,8 @@ SHOW transaction_deferrable;
  on
 (1 row)
 
-INSERT INTO abc VALUES (1);
-INSERT INTO abc VALUES (2);
+INSERT INTO txn_table VALUES (1);
+INSERT INTO txn_table VALUES (2);
 COMMIT AND CHAIN;  -- TBLOCK_END
 SHOW transaction_isolation;
  transaction_isolation 
@@ -741,11 +741,11 @@ SHOW transaction_deferrable;
  on
 (1 row)
 
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
 ERROR:  invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
-INSERT INTO abc VALUES (3);  -- check it's really aborted
+LINE 1: INSERT INTO txn_table VALUES ('error');
+  ^
+INSERT INTO txn_table VALUES (3);  -- check it's really aborted
 ERROR:  current transaction is aborted, commands ignored until end of transaction block
 COMMIT AND CHAIN;  -- TBLOCK_ABORT_END
 SHOW transaction_isolation;
@@ -766,7 +766,7 @@ SHOW transaction_deferrable;
  on
 (1 row)
 
-INSERT INTO abc VALUES (4);
+INSERT INTO txn_table VALUES (4);
 COMMIT;
 START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
 SHOW transaction_isolation;
@@ -788,10 +788,10 @@ SHOW transaction_deferrable;
 (1 row)
 
 SAVEPOINT x;
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
 ERROR:  invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
+LINE 1: INSERT INTO txn_table VALUES ('error');
+  ^
 COMMIT AND CHAIN;  -- TBLOCK_ABORT_PENDING
 SHOW transaction_isolation;
  transaction_isolation 
@@ -811,7 +811,7 @@ SHOW transaction_deferrable;
  on
 (1 row)
 
-INSERT INTO abc VALUES (5);
+INSERT INTO txn_table VALUES (5);
 COMMIT;
 START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
 SHOW transaction_isolation;
@@ -873,7 +873,7 @@ SHOW transaction_deferrable;
  off
 (1 row)
 
-INSERT INTO abc VALUES (6);
+INSERT INTO txn_table VALUES (6);
 ROLLBACK AND CHAIN;  -- TBLOCK_ABORT_PENDING
 SHOW transaction_isolation;
  transaction_isolation 
@@ -893,10 +893,10 @@ SHOW transaction_deferrable;
  off
 (1 row)
 
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
 ERROR:  invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
+LINE 1: INSERT INTO txn_table VALUES ('error');
+  ^
 ROLLBACK AND CHAIN;  -- TBLOCK_ABORT_END
 SHOW transaction_isolation;
  

Re: Autogenerate some wait events code and documentation

[Michael Paquier]

On Mon, May 15, 2023 at 06:45:23PM +0200, Drouvot, Bertrand wrote:
> On 5/6/23 4:37 AM, Michael Paquier wrote:
>> On Sat, May 06, 2023 at 11:23:17AM +0900, Michael Paquier wrote:
 I'll look at v7 when the v17 branch opens and propose the separate patch
 mentioned above at that time too.
>>> 
>>> Thanks, again.
>> 
>> By the way, while browsing through the patch, I have noticed two
>> things:
>> - The ordering of the items for Lock and LWLock is incorrect.
> 
> Oh right, fixed in V8 attached (moved the sort on the third column
> instead of the second which has always the same content "WAIT_EVENT_DOCONLY"
> for Lock and LWLock).

Ah, I didn't notice that.  Makes sense.

>> - We are missing some of the LWLock entries, like CommitTsBuffer,
>> XactBuffer or WALInsert, as of all the entries in
>> BuiltinTrancheNames.
> 
> Yeah, my bad. Fixed in V8 attached.

BufFileTruncate and BufFileWrite have an incorrect order in HEAD's
monitoring.sgml (will fix in a minute for 16~).  So your patch fixes
that.

PgStatsDSA and PgStatsData are reversed in your patch compared to
HEAD, actually, based on the way perl sees fit to do its ordering by
giving priority to upper-case characters.  Same for RelCacheInit and
RelationMapping, or even SInvalRead/SInvalWrite being now before the
"Serial" family.  Worse, the tables LWLock and Lock are in an
incorrect order as well with the patch.  We'd better be a bit more
verbose with the sort step, I think..  perl does not handle well
sorting with collations from what I recall, but we could use uc() with
a block sort to force the operation to be case-insensitive, like "sort
{uc($a) cmp uc($b)}".  That needs to be applied here, I guess: 
+# Sort the lines based on the third column
+my @lines_sorted =
+  sort { (split(/\t/, $a))[2] cmp(split(/\t/, $b))[2] } @lines;

And it looks like you'd need to apply uc() on each [2] element.  I
would add a comment about this detail, as well.

No entries are missing, after comparing what's generated by the patch
with the contents of HEAD.

Small nit-ish question: waiteventnames.sgml or wait_event_types.sgml?
Same for generate-waiteventtypes.pl?

>> My apologies for not noticing that earlier.  This exists in v6 as much
>> as v7.
> 
> No problem at all and thanks for the call out!

FWIW, I would have posted two patches, one with the refactoring of
done in [1], and a second that switches to the automation, to make
clear the preparatory step.

[1]: 
https://www.postgresql.org/message-id/c6f35117-4b20-4c78-1df5-d3056010d...@gmail.com
--
Michael


signature.asc
Description: PGP signature

Re: running logical replication as the subscription owner

[Amit Kapila]

On Mon, May 15, 2023 at 7:24 PM Jelte Fennema  wrote:
>
> On Mon, 15 May 2023 at 14:47, Masahiko Sawada  wrote:
> > Thank you for the patch! I think we might want to have tests for it.
>
> Yes, code looks good. But indeed some tests would be great. It seems
> we forgot to actually do:
>

Agreed with you and Sawada-San about having a test. BTW, shall we
slightly tweak the documentation [1]: "The subscription apply process
will, at a session level, run with the privileges of the subscription
owner. However, when performing an insert, update, delete, or truncate
operation on a particular table, it will switch roles to the table
owner and perform the operation with the table owner's privileges." to
be bit more specific about initial sync process as well?

[1] - https://www.postgresql.org/docs/devel/logical-replication-security.html

-- 
With Regards,
Amit Kapila.

Re: running logical replication as the subscription owner

[Amit Kapila]

On Fri, May 12, 2023 at 5:25 PM Ajin Cherian  wrote:
>
> On Fri, May 12, 2023 at 1:49 PM Amit Kapila  wrote:
> >
>
> I tried the following test:
>
> 
> Repeat On the publisher and subscriber:
>  /* Create role regress_alice with  NOSUPERUSER on
>publisher and subscriber and a table for replication */
>
> CREATE ROLE regress_alice NOSUPERUSER LOGIN;
> CREATE ROLE regress_admin SUPERUSER LOGIN;
> GRANT CREATE ON DATABASE postgres TO regress_alice;
> SET SESSION AUTHORIZATION regress_alice;
> CREATE SCHEMA alice;
> GRANT USAGE ON SCHEMA alice TO regress_admin;
> CREATE TABLE alice.test (i INTEGER);
> ALTER TABLE alice.test REPLICA IDENTITY FULL;
>

Why do we need a schema and following grant statement for this test?

> On the publisher:
> postgres=> insert into alice.test values(1);
> postgres=> insert into alice.test values(2);
> postgres=> insert into alice.test values(3);
> postgres=> CREATE PUBLICATION alice FOR TABLE alice.test
> WITH (publish_via_partition_root = true);
>

Again, 'publish_via_partition_root' doesn't seem to be required. Let's
try to write a minimal test for the initial sync behaviour.

-- 
With Regards,
Amit Kapila.

Re: Move un-parenthesized syntax docs to "compatibility" for few SQL commands

[Michael Paquier]

On Mon, May 15, 2023 at 12:36:51PM -0700, Nathan Bossart wrote:
> There's still some time before we'll be able to commit any of these, but
> here is an attempt at addressing all the feedback thus far.

-   The parenthesized syntax was added in
-   PostgreSQL 9.0;  the unparenthesized
-   syntax is deprecated.
[...]
+   | CLUSTER '(' utility_option_list ')'
+   {
+   ClusterStmt *n = makeNode(ClusterStmt);
+
+   n->relation = NULL;
+   n->indexname = NULL;
+   n->params = $3;
+   $$ = (Node *) n;
+   }

Hmm.  This is older than the oldest version we have to support for
pg_upgrade and co.  Would it be worth switching clusterdb to use the
parenthesized grammar, adding on the way a test for this new grammar
flavor without a table in the TAP tests (too costly for the main
regression test suite)?
--
Michael


signature.asc
Description: PGP signature

Re: walsender performance regression due to logical decoding on standby changes

[Drouvot, Bertrand]

Hi,

On 5/15/23 4:39 PM, Bharath Rupireddy wrote:

On Mon, May 15, 2023 at 6:14 PM Masahiko Sawada  wrote:


On Mon, May 15, 2023 at 1:49 PM Thomas Munro  wrote:


Do we need to add an open item for this issue in
https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items? If yes, can
anyone in this loop add one?


I do think we need one for this issue and then just added it.

Regards,

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

Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN

[Michael Paquier]

On Mon, May 15, 2023 at 10:07:04AM +0200, Drouvot, Bertrand wrote:
> This is preliminary work to autogenerate some wait events
> code and documentation done in [1].
> 
> The patch introduces 2 new "wait events" (WAIT_EVENT_EXTENSION
> and WAIT_EVENT_BUFFER_PIN) and their associated functions
> (pgstat_get_wait_extension() and pgstat_get_wait_bufferpin() resp.)
> 
> Please note that that would not break extensions outside contrib
> that make use of the existing PG_WAIT_EXTENSION.

I have looked at this one, and I think that's OK for what you are
aiming at here (in addition to my previous message that I hope
provides enough context about the whys and the hows).
--
Michael


signature.asc
Description: PGP signature

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

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thanks for reviewing! PSA new version patchset.

> 1. get_logical_slot_infos_per_db
> 
> I noticed that the way this is coded, 'ntups' and 'num_slots' seems to
> have exactly the same meaning. IMO you can simplify this by removing
> 'ntups'.
> 
> BEFORE
> + int ntups;
> + int num_slots = 0;
> 
> SUGGESTION
> + int num_slots;
> 
> ~
> 
> BEFORE
> + ntups = PQntuples(res);
> +
> + if (ntups)
> + {
> 
> SUGGESTION
> + num_slots = PQntuples(res);
> +
> + if (num_slots)
> + {
> 
> ~
> 
> BEFORE
> + slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) * ntups);
> 
> SUGGESTION
> + slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) *
> num_slots);
> 
> ~
> 
> BEFORE
> + for (slotnum = 0; slotnum < ntups; slotnum++)
> + {
> + LogicalSlotInfo *curr = [num_slots++];
> 
> SUGGESTION
> + for (slotnum = 0; slotnum < ntups; slotnum++)
> + {
> + LogicalSlotInfo *curr = [slotnum];

Right, fixed.

> 2. get_logical_slot_infos, print_slot_infos
> 
> > >
> > > In another thread [1] I am posting some minor patch changes to the
> > > VERBOSE logging (changes to double-quotes and commas etc.). Please
> > > keep a watch on that thread because if gets pushed then this one will
> > > be impacted. e.g. your logging here ought also to include the same
> > > suggested double quotes.
> >
> > I thought it would be pushed soon, so the suggestion was included.
> 
> OK, but I think you have accidentally missed adding similar new double
> quotes to all other VERBOSE logging in your patch.
> 
> e.g. see get_logical_slot_infos:
> pg_log(PG_VER
BOSE, "Database: %s", pDbInfo->db_name);
> 

Oh, I missed it. Fixed. I grepped patches and could not find other lines
which should be double-quoted.

In addition, I ran pgindent again for 0001.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED


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


v14-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description:  v14-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch


v14-0003-pg_upgrade-Add-check-function-for-include-logica.patch
Description:  v14-0003-pg_upgrade-Add-check-function-for-include-logica.patch


v14-0004-Change-the-method-used-to-check-logical-replicat.patch
Description:  v14-0004-Change-the-method-used-to-check-logical-replicat.patch

Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN

[Drouvot, Bertrand]

Hi,

On 5/16/23 7:14 AM, Michael Paquier wrote:

On Mon, May 15, 2023 at 06:01:02PM -0700, Andres Freund wrote:

On 2023-05-16 09:38:54 +0900, Michael Paquier wrote:

On Mon, May 15, 2023 at 05:17:16PM -0700, Andres Freund wrote:



These are the two things refactored in the patch, explaining the what.
The reason behind the why is to make the script in charge of
generating all these structures and functions consistent for all the
wait event classes, simply.  Treating all the wait event classes
together eases greatly the generation of the documentation, so that it
is possible to enforce an ordering of the tables of each class used to
list each wait event type attached to them.


Right, it does "fix" the ordering issue (for BufferPin and Extension)
that I've described in the patch introduction in [1]:

"
  so that PG_WAIT_LWLOCK, PG_WAIT_LOCK, PG_WAIT_BUFFER_PIN and 
PG_WAIT_EXTENSION are not autogenerated.


This result to having the wait event part of the documentation "monitoring-stats" not 
ordered as compared to the "Wait Event Types" Table.
.
.
.
"

Thanks Michael for having provided this detailed explanation (my patch
introduction clearly was missing some context as Andres pointed out).

[1]: 
https://www.postgresql.org/message-id/77a86b3a-c4a8-5f5d-69b9-d70bbf2e9b98%40gmail.com

Regards,

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