Re: Doc: vcregress .bat commands list lacks "taptest"

[Michael Paquier]

On Mon, Sep 25, 2023 at 03:32:04PM +0900, Yugo NAGATA wrote:
> I found that "taptest" is missing in vcregress.bat command list in the
> documentation when I tested psql and pgbench on Windows. I know there
> is a plan to remove MSVC scripts[1], but is it worth adding one line
> to the list for now?
> 
> [1] https://www.postgresql.org/message-id/flat/ZQzp_VMJcerM1Cs_%40paquier.xyz
> 
> I attached a patch for this case.

Assuming that the MSVC scripts are gone in v17, and assuming that your
suggestion is backpatched, it would still be useful for 5 years until
v16 is EOL'd.  So I would say yes, but with a backpatch.
--
Michael


signature.asc
Description: PGP signature

Re: Clarify where the severity level is defined

[Daniel Gustafsson]

> On 25 Sep 2023, at 08:22, Kuwamura Masaki  
> wrote:

> Recently I read the document about ereport()[1].
> Then, I felt that there is little information about severity level.
> So I guess it can be kind to clarify where severity level is defined(see 
> attached patch please).

That makes sense, we already point to other related files on that page so this
is line with that.

--
Daniel Gustafsson

RE: logical decoding and replication of sequences, take 2

[Zhijie Hou (Fujitsu)]

On Friday, September 15, 2023 11:11 AM Zhijie Hou (Fujitsu) 
 wrote:
> 
> On Wednesday, August 16, 2023 10:27 PM Tomas Vondra
>  wrote:
> 
> Hi,
> 
> >
> >
> > I guess we could update the origin, per attached 0004. We don't have
> > timestamp to set replorigin_session_origin_timestamp, but it seems we
> > don't need that.
> >
> > The attached patch merges the earlier improvements, except for the
> > part that experimented with adding a "fake" transaction (which turned
> > out to have a number of difficult issues).
> 
> I tried to test the patch and found a crash when calling
> pg_logical_slot_get_changes() to consume sequence changes.

Oh, after confirming again, I realize it's my fault that my build environment
was not clean. This case passed after rebuilding. Sorry for the noise.

Best Regards,
Hou zj

Doc: vcregress .bat commands list lacks "taptest"

[Yugo NAGATA]

Hi,

I found that "taptest" is missing in vcregress.bat command list in the
documentation when I tested psql and pgbench on Windows. I know there
is a plan to remove MSVC scripts[1], but is it worth adding one line
to the list for now?

[1] https://www.postgresql.org/message-id/flat/ZQzp_VMJcerM1Cs_%40paquier.xyz

I attached a patch for this case.

Regards,
Yugo Nagata

-- 
Yugo NAGATA 
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 379a2ea80b..435a91228a 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -463,6 +463,7 @@ $ENV{CONFIG}="Debug";
 vcregress isolationcheck
 vcregress bincheck
 vcregress recoverycheck
+vcregress taptest
 
 
To change the schedule used (default is parallel), append it to the
@@ -477,8 +478,9 @@ $ENV{CONFIG}="Debug";
 
   
Running the regression tests on client programs, with
-   vcregress bincheck, or on recovery tests, with
-   vcregress recoverycheck, requires an additional Perl module
+   vcregress bincheck, on recovery tests, with
+   vcregress recoverycheck, or TAP tests specified with
+   vcregress taptest requires an additional Perl module
to be installed:

 

Re: Make --help output fit within 80 columns per line

[torikoshia]

On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane  
wrote:

Thanks for your investigation!

On Fri, Sep 15, 2023 at 11:11 AM torikoshia 
 wrote:

I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?


Well, that's the question du jour, isn't it? The 80 character limit is 
based on punch cards, and really has no place in modern systems. While 
gnu systems are stuck in the past, many other ones have moved on to 
more sensible defaults:


$ wget --help | wc -L
110

$ gcloud --help | wc -L
122

$ yum --help | wc -L
122

git is an interesting one, as they force things through a pager for 
their help, but if you look at their raw help text files, they have 
plenty of times they go past 80 when needed:


$ wc -L git/Documentation/git-*.txt | sort -g | tail -20
109 git-filter-branch.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txt

So in summary, I think 80 is a decent soft limit, but let's not stress 
out about some lines going over that, and make a hard limit of perhaps 
120.


+1. It may be a good compromise.
For enforcing the hard limit, is it better to add a regression test like 
patch 0001?



On 2023-09-21 16:45, Peter Eisentraut wrote:

On 31.08.23 09:47, torikoshia wrote:
BTW, psql --help outputs the content of PGHOST, which caused a failure 
in the test:


```
-h, --host=HOSTNAME  database server host or socket directory
  (default: 
"/var/folders/m7/9snkd5b54cx_b4lxkl9ljlccgn/T/LobrmSUf7t")

```

It may be overkill, added a logic for removing the content of PGHOST.


I wonder, should we remove this?  We display the
environment-variable-based defaults in psql --help, but not for any
other programs.  This is potentially misleading.


Agreed. It seems inconsistent with other commands.
Patch 0002 removed environment-variable-based defaults in psql --help.

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group CorporationFrom b0ae0826374ce86c95ee36637913b65f865d6e0b Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 25 Sep 2023 10:40:36 +0900
Subject: [PATCH v1 1/2] Added a test for checking the line length of --help
 output.

---
 src/test/perl/PostgreSQL/Test/Utils.pm | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index d66fe1cf45..291b5dcbbb 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -884,6 +884,14 @@ sub program_help_ok
 	ok($result, "$cmd --help exit code 0");
 	isnt($stdout, '', "$cmd --help goes to stdout");
 	is($stderr, '', "$cmd --help nothing to stderr");
+
+	# We set the hard limit on the length of line to 120
+	subtest "$cmd --help outputs fit within 120 columns per line" => sub {
+	foreach my $line (split /\n/, $stdout)
+	{
+		ok(length($line) <= 120, "$line");
+	}
+};
 	return;
 }
 
-- 
2.39.2

From 207c4059b42e975bc788452838099da825972d15 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 25 Sep 2023 10:52:08 +0900
Subject: [PATCH v1 2/2] Removed environment-variable-based defaults in psql --help

---
 src/bin/psql/help.c | 33 -
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 12280c0e54..3b2d59e2ee 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -50,22 +50,10 @@
 void
 usage(unsigned short int pager)
 {
-	const char *env;
-	const char *user;
-	char	   *errstr;
 	PQExpBufferData buf;
 	int			nlcount;
 	FILE	   *output;
 
-	/* Find default user, in case we need it. */
-	user = getenv("PGUSER");
-	if (!user)
-	{
-		user = get_user_name(&errstr);
-		if (!user)
-			pg_fatal("%s", errstr);
-	}
-
 	/*
 	 * To avoid counting the output lines manually, build the output in "buf"
 	 * and then count them.
@@ -77,13 +65,8 @@ usage(unsigned short int pager)
 	HELP0("  psql [OPTION]... [DBNAME [USERNAME]]\n\n");
 
 	HELP0("General options:\n");
-	/* Display default database */
-	env = getenv("PGDATABASE");
-	if (!env)
-		env = user;
 	HELP0("  -c, --command=COMMANDrun only single command (SQL or internal) and exit\n");
-	HELPN("  -d, --dbname=DBNAME  database name to connect to (default: \"%s\")\n",
-		  env);
+	HELP0("  -d, --dbname=DBNAME  database name to connect to\n");
 	HELP0("  -f, --file=FILENAME  execute commands from file, then exit\n");
 	HELP0("  -l, --list   list available databases, then exit\n");
 	HELP0("  -v, --set=, --variable=NAME=VALUE\n"
@@ -128,17 +111,9 @@ 

Clarify where the severity level is defined

[Kuwamura Masaki]

Hi,

Recently I read the document about ereport()[1].
Then, I felt that there is little information about severity level.
So I guess it can be kind to clarify where severity level is defined(see
attached patch please).

Any thoughts?


[1] https://www.postgresql.org/docs/devel/error-message-reporting.html
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 514090d5a6..738fd3fe0c 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -105,7 +105,8 @@ less -x4
 

 There are two required elements for every message: a severity level
-(ranging from DEBUG to PANIC) and a 
primary
+(ranging from DEBUG to PANIC, 
defined at
+the top of src/include/utils/elog.h) and a primary
 message text.  In addition there are optional elements, the most
 common of which is an error identifier code that follows the SQL spec's
 SQLSTATE conventions.

Re: pg_upgrade and logical replication

[Michael Paquier]

On Mon, Sep 25, 2023 at 10:05:41AM +0530, Amit Kapila wrote:
> I also don't think that this patch has to solve the problem of
> publishers in any way but as per my understanding, if due to some
> reason we are not able to do the upgrade of publishers, this can add
> more steps for users than they have to do now for logical replication
> set up after upgrade. This is because now after restoring the
> subscription rel's and origin, as soon as we start replication after
> creating the slots on the publisher, we will never be able to
> guarantee data consistency. So, they need to drop the entire
> subscription setup including truncating the relations, and then set it
> up from scratch which also means they need to somehow remember or take
> a dump of the current subscription setup. According to me, the key
> point is to have a mechanism to set up slots correctly to allow
> replication (or subscriptions) to work after the upgrade. Without
> that, it appears to me that we are restoring a subscription where it
> can start from some random LSN and can easily lead to data consistency
> issues where it can miss some of the updates.

Sure, that's assuming that the publisher side is upgraded.  FWIW, my
take is that there's room to move forward with this patch anyway in
favor of cases like rollover upgrades to the subscriber.

> This is the primary reason why I prioritized to work on the publisher
> side before getting this patch done, otherwise, the solution for this
> patch was relatively clear. I am not sure but I guess this could be
> the reason why originally we left it in the current state, otherwise,
> restoring subscription rel's or origin doesn't seem to be too much of
> an additional effort than what we are doing now.

By "additional effort", you are referring to what the patch is doing,
with the binary dump of pg_subscription_rel, right?
--
Michael


signature.asc
Description: PGP signature

Re: Document efficient self-joins / UPDATE LIMIT techniques.

[jian he]

Hi.
-
In cases where a DML operation involving many rows must be performed,
and that table experiences numerous other simultaneous DML operations,
a FOR UPDATE clause used in conjunction with SKIP LOCKED can be useful
for performing partial DML operations:

WITH mods AS (SELECT ctid FROM mytable
  WHERE status = 'active' AND retries > 10
  ORDER BY id FOR UPDATE SKIP LOCKED)
UPDATE mytable SET status = 'failed'
FROM mods WHERE mytable.ctid = mods.ctid

This allows the DML operation to be performed in parts, avoiding
locking, until such time as the set of rows that remain to be modified
is small enough that the locking will not affect overall performance,
at which point the same statement can be issued without the SKIP
LOCKED clause to ensure that no rows were overlooked.
--
mods found out the ctids to be updated, update mytable actually do the update.
I didn't get "This allows the DML operation to be performed in parts".

omit "at which point", the last sentence still makes sense. so I
didn't get "at which point"?

I am not native english speaker.

Re: pg_upgrade and logical replication

[Michael Paquier]

On Mon, Sep 25, 2023 at 05:35:18AM +, Hayato Kuroda (Fujitsu) wrote:
> Personally, I prefer to change max_logical_replication_workers. Mainly there 
> are
> two reasons:
> 
> 1. Your approach must be back-patched to older versions which support logical
>replication feature, but the oldest one (PG10) has already been 
> unsupported.
>We should not modify such a branch.

This suggestion would be only for HEAD as it changes the behavior of -b.

> 2. Also, "max_logical_replication_workers = 0" approach would be consistent
>with what we are doing now and for upgrade of publisher patch.
>Please see the previous discussion [1].

Yeah, you're right.  Consistency would be good across the board, and
we'd need to take care of the old clusters as well, so the GUC
enforcement would be needed as well.  It does not strike me that this
extra IsBinaryUpgrade would hurt anyway?  Forcing the hand of the
backend has the merit of allowing the removal of the tweak with
max_logical_replication_workers at some point in the future.
--
Michael


signature.asc
Description: PGP signature

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Ryoga Yoshida]

On 2023-09-25 14:38, Michael Paquier wrote:

We would not wait on the lock if force=false, which would do
nowait=true.  And !force reads the same to me as force=false.

Anyway, I am OK to remove this part.  That seems to confuse you, so
you may not be the only one who would read this comment.


When I first read it, I didn't read that !force as force=false, so 
removing it might be better.



Another idea would be to do like in pgstat.c by adding the following
line, then use "nowait" to call each sub-function:
nowait = !force;
pgstat_flush_wal(nowait);
pgstat_flush_io(nowait);


That's very clear and I think it's good.

Ryoga Yoshida

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

[Bharath Rupireddy]

On Fri, Sep 22, 2023 at 12:11 PM Amit Kapila  wrote:
>
> Yeah, both by tests and manually verifying the WAL records. Basically,
> we need to care about records that could be generated by background
> processes like checkpointer/bgwriter or can be generated during system
> table scans. You may want to read my latest email for a summary on how
> we reached at this design choice [1].
>
> [1] - 
> https://www.postgresql.org/message-id/CAA4eK1JVKZGRHLOEotWi%2Be%2B09jucNedqpkkc-Do4dh5FTAU%2B5w%40mail.gmail.com

+/* Logical slots can be migrated since PG17. */
+if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+{

Why can't the patch allow migration of logical replication slots from
PG versions < 17 to say 17 or later? If done, it will be a main
advantage of the patch since it will enable seamless major version
upgrades of postgres database instances with logical replication
slots.

I'm looking at the changes to the postgres backend that this patch
does - AFICS, it does 2 things 1) implements
binary_upgrade_validate_wal_logical_end function, 2) adds an assertion
that the logical slots won't get invalidated. For (1), pg_upgrade can
itself can read the WAL from the old cluster to determine the logical
WAL end (i.e. implement the functionality of
binary_upgrade_validate_wal_logical_end ) because the xlogreader is
available to FRONTEND tools. For (2), it's just an assertion and
logical WAL end determining logic will anyway determine whether or not
the slots are valid; if needed, the assertion can be backported.

Is there anything else that stops this patch from supporting migration
of logical replication slots from PG versions < 17?

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

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Michael Paquier]

On Mon, Sep 25, 2023 at 02:16:22PM +0900, Ryoga Yoshida wrote:
> On 2023-09-25 12:47, Michael Paquier wrote:
> in attached file
>> +/* like in pgstat.c, don't wait for lock acquisition when !force */
> 
> Isn't it the case with force=true and !force that it doesn't wait for the
> lock acquisition.  In fact, force may be false.

We would not wait on the lock if force=false, which would do
nowait=true.  And !force reads the same to me as force=false.

Anyway, I am OK to remove this part.  That seems to confuse you, so
you may not be the only one who would read this comment.

Another idea would be to do like in pgstat.c by adding the following
line, then use "nowait" to call each sub-function:
nowait = !force;
pgstat_flush_wal(nowait);
pgstat_flush_io(nowait);
--
Michael


signature.asc
Description: PGP signature

RE: pg_upgrade and logical replication

[Hayato Kuroda (Fujitsu)]

Dear Michael,

> I'd like to propose a check
> for IsBinaryUpgrade into ApplyLauncherRegister() instead as it makes
> no real sense to start apply workers in this context.  That would be
> equivalent to max_logical_replication_workers = 0.

Personally, I prefer to change max_logical_replication_workers. Mainly there are
two reasons:

1. Your approach must be back-patched to older versions which support logical
   replication feature, but the oldest one (PG10) has already been unsupported.
   We should not modify such a branch.
2. Also, "max_logical_replication_workers = 0" approach would be consistent
   with what we are doing now and for upgrade of publisher patch.
   Please see the previous discussion [1].

[1]: 
https://www.postgresql.org/message-id/CAA4eK1%2BWBphnmvMpjrxceymzuoMuyV2_pMGaJq-zNODiJqAa7Q%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: Row pattern recognition

[Tatsuo Ishii]

> On Fri, Sep 22, 2023, 3:13 AM Tatsuo Ishii  wrote:
> 
>> > Op 9/22/23 om 07:16 schreef Tatsuo Ishii:
>> >>> Attached is the fix against v6 patch. I will include this in upcoming
>> >>> v7 patch.
>> >> Attached is the v7 patch. It includes the fix mentioned above.  Also
>> > (Champion's address bounced; removed)
>>
>> On my side his adress bounced too:-<
>>
> 
> Yep. I'm still here, just lurking for now. It'll take a little time for me
> to get back to this thread, as my schedule has changed significantly. :D

Hope you get back soon...

By the way, I was thinking about eliminating recusrive calls in
pattern matching. Attached is the first cut of the implementation. In
the attached v8 patch:

- No recursive calls anymore. search_str_set_recurse was removed.

- Instead it generates all possible pattern variable name initial
  strings before pattern matching. Suppose we have "ab" (row 0) and
  "ac" (row 1). "a" and "b" represents the pattern variable names
  which are evaluated to true.  In this case it will generate "aa",
  "ac", "ba" and "bc" and they are examined by do_pattern_match one by
  one, which performs pattern matching.

- To implement this, an infrastructure string_set* are created. They
  take care of set of StringInfo.

I found that the previous implementation did not search all possible
cases. I believe the bug is fixed in v8.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
>From 281ee5c3eae85f96783422cb2f9987c3af8c8a68 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii 
Date: Mon, 25 Sep 2023 14:01:14 +0900
Subject: [PATCH v8 1/7] Row pattern recognition patch for raw parser.

---
 src/backend/parser/gram.y   | 222 ++--
 src/include/nodes/parsenodes.h  |  56 
 src/include/parser/kwlist.h |   8 ++
 src/include/parser/parse_node.h |   1 +
 4 files changed, 273 insertions(+), 14 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7d2032885e..74c2069050 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	DefElem	   *defelt;
 	SortBy	   *sortby;
 	WindowDef  *windef;
+	RPCommonSyntax	*rpcom;
+	RPSubsetItem	*rpsubset;
 	JoinExpr   *jexpr;
 	IndexElem  *ielem;
 	StatsElem  *selem;
@@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	MergeWhenClause *mergewhen;
 	struct KeyActions *keyactions;
 	struct KeyAction *keyaction;
+	RPSkipTo	skipto;
 }
 
 %type 	stmt toplevel_stmt schema_stmt routine_body_stmt
@@ -453,8 +456,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 TriggerTransitions TriggerReferencing
 vacuum_relation_list opt_vacuum_relation_list
 drop_option_list pub_obj_list
-
-%type 	opt_routine_body
+row_pattern_measure_list row_pattern_definition_list
+opt_row_pattern_subset_clause
+row_pattern_subset_list row_pattern_subset_rhs
+row_pattern
+%type 	 row_pattern_subset_item
+%type 	opt_routine_body row_pattern_term
 %type  group_clause
 %type 	group_by_list
 %type 	group_by_item empty_grouping_set rollup_clause cube_clause
@@ -551,6 +558,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	relation_expr_opt_alias
 %type 	tablesample_clause opt_repeatable_clause
 %type 	target_el set_target insert_column_item
+row_pattern_measure_item row_pattern_definition
+%type 	first_or_last
 
 %type 		generic_option_name
 %type 	generic_option_arg
@@ -633,6 +642,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	window_clause window_definition_list opt_partition_clause
 %type 	window_definition over_clause window_specification
 opt_frame_clause frame_extent frame_bound
+%type 	opt_row_pattern_common_syntax opt_row_pattern_skip_to
+%type 	opt_row_pattern_initial_or_seek
+%type 	opt_row_pattern_measures
 %type 	opt_window_exclusion_clause
 %type 		opt_existing_window_name
 %type  opt_if_not_exists
@@ -659,7 +671,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 json_object_constructor_null_clause_opt
 json_array_constructor_null_clause_opt
 
-
 /*
  * Non-keyword token types.  These are hard-wired into the "flex" lexer.
  * They must be listed first so that their numeric codes do not depend on
@@ -702,7 +713,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE
 
 	DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS
-	DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
+	DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC
 	DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P
 	DOUBLE_P DROP
 
@@ -718,7 +729,7 @@ sta

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Ryoga Yoshida]

On 2023-09-25 12:47, Michael Paquier wrote:
in attached file

+   /* like in pgstat.c, don't wait for lock acquisition when !force */


Isn't it the case with force=true and !force that it doesn't wait for 
the lock acquisition.  In fact, force may be false.


Ryoga Yoshida

Re: SQL:2011 application time

[jian he]

On Wed, Sep 20, 2023 at 10:50 AM Paul Jungwirth
 wrote:
>
> On 9/17/23 20:11, jian he wrote:
> > small issues so far I found, v14.
>
> Thank you again for the review! v15 is attached.
>

hi. some tiny issues.
IN src/backend/utils/adt/ri_triggers.c

else {
appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x",
pk_only, pkrelname);
}
should change to

else
{
appendStringInfo(&querybuf, "SELECT 1 FROM %s%s x",
pk_only, pkrelname);
}


It would be better, we mention it somewhere:
by default, you can only have a primary key(range_type[...],
range_type WITHOUT OVERLAPS).

preceding without overlaps, all columns (in primary key) data types
only allowed range types.
---
The WITHOUT OVERLAPS value must be a range type and is used to
constrain the record's applicability to just that interval (usually a
range of dates or timestamps).

"interval", I think "period" or "range" would be better. I am not sure
we need to mention " must be a range type, not a multi range type".
-
I just `git apply`, then ran the test, and one test failed. Some minor
changes need to make the test pass.

Re: pg_upgrade and logical replication

[Amit Kapila]

On Fri, Sep 22, 2023 at 4:36 AM Michael Paquier  wrote:
>
> On Thu, Sep 21, 2023 at 02:35:55PM +0530, Amit Kapila wrote:
> > It is because after upgrade of both publisher and subscriber, the
> > subscriptions won't work. Both publisher and subscriber should work,
> > otherwise, the logical replication set up won't work. I think we can
> > probably do this, if we can document clearly how the user can make
> > their logical replication set up work after upgrade.
>
> Yeah, well, this comes back to my original point that the upgrade of
> publisher nodes and subscriber nodes should be treated as two
> different problems or we're mixing apples and oranges (and a node
> could have both subscriber and publishers).  While being able to
> support both is a must, it is going to be a two-step process at the
> end, with the subscribers done first and the publishers done after.
> That's also kind of the point that Julien makes in top message of this
> thread.
>
> I agree that docs are lacking in the proposed patch in terms of
> restrictions, assumptions and process flow, but taken in isolation the
> problem of the publishers is not something that this patch has to take
> care of.
>

I also don't think that this patch has to solve the problem of
publishers in any way but as per my understanding, if due to some
reason we are not able to do the upgrade of publishers, this can add
more steps for users than they have to do now for logical replication
set up after upgrade. This is because now after restoring the
subscription rel's and origin, as soon as we start replication after
creating the slots on the publisher, we will never be able to
guarantee data consistency. So, they need to drop the entire
subscription setup including truncating the relations, and then set it
up from scratch which also means they need to somehow remember or take
a dump of the current subscription setup. According to me, the key
point is to have a mechanism to set up slots correctly to allow
replication (or subscriptions) to work after the upgrade. Without
that, it appears to me that we are restoring a subscription where it
can start from some random LSN and can easily lead to data consistency
issues where it can miss some of the updates.

This is the primary reason why I prioritized to work on the publisher
side before getting this patch done, otherwise, the solution for this
patch was relatively clear. I am not sure but I guess this could be
the reason why originally we left it in the current state, otherwise,
restoring subscription rel's or origin doesn't seem to be too much of
an additional effort than what we are doing now.

-- 
With Regards,
Amit Kapila.

Re: Postgres picks suboptimal index after building of an extended statistics

[Andrey Lepikhov]

On 12/8/2021 06:26, Tomas Vondra wrote:

On 8/11/21 2:48 AM, Peter Geoghegan wrote:

On Wed, Jun 23, 2021 at 7:19 AM Andrey V. Lepikhov
 wrote:

Ivan Frolkov reported a problem with choosing a non-optimal index during
a query optimization. This problem appeared after building of an
extended statistics.


Any thoughts on this, Tomas?



Thanks for reminding me, I missed / forgot about this thread.

I agree the current behavior is unfortunate, but I'm not convinced the 
proposed patch is fixing the right place - doesn't this mean the index 
costing won't match the row estimates displayed by EXPLAIN?


I wonder if we should teach clauselist_selectivity about UNIQUE indexes, 
and improve the cardinality estimates directly, not just costing for 
index scans.


Also, is it correct that the patch calculates num_sa_scans only when 
(numIndexTuples >= 0.0)?
I can't stop thinking about this issue. It is bizarre when Postgres 
chooses a non-unique index if a unique index gives us proof of minimum scan.
I don't see a reason to teach the clauselist_selectivity() routine to 
estimate UNIQUE indexes. We add some cycles, but it will work with btree 
indexes only.
Maybe to change compare_path_costs_fuzzily() and add some heuristic, for 
example:
"If selectivity of both paths gives us no more than 1 row, prefer to use 
a unique index or an index with least selectivity."


--
regards,
Andrey Lepikhov
Postgres Professional

Re: Synchronizing slots from primary to standby

[shveta malik]

On Fri, Sep 22, 2023 at 3:48 PM Amit Kapila  wrote:
>
> On Thu, Sep 21, 2023 at 9:16 AM shveta malik  wrote:
> >
> > On Tue, Sep 19, 2023 at 10:29 AM shveta malik  
> > wrote:
> >
> > Currently in patch001, synchronize_slot_names is a GUC on both primary
> > and physical standby. This GUC tells which all logical slots need to
> > be synced on physical standbys from the primary. Ideally it should be
> > a GUC on physical standby alone and each physical standby should be
> > able to communicate the value to the primary (considering the value
> > may vary for different physical replicas of the same primary). The
> > primary on the other hand should be able to take UNION of these values
> > and let the logical walsenders (belonging to the slots in UNION
> > synchronize_slots_names) wait for physical standbys for confirmation
> > before sending those changes to logical subscribers. The intent is
> > logical subscribers should never be ahead of physical standbys.
> >
>
> Before getting into the details of 'synchronize_slot_names', I would
> like to know whether we really need the second GUC
> 'standby_slot_names'. Can't we simply allow all the logical wal
> senders corresponding to 'synchronize_slot_names' to wait for just the
> physical standby(s) (physical slot corresponding to such physical
> standby) that have sent ' synchronize_slot_names'list? We should have
> one physical standby slot corresponding to one physical standby.
>

yes, with the new approach (to be implemented next) where we plan to
send synchronize_slot_names from each physical standby to primary, the
standby_slot_names GUC should no longer be needed on primary. The
physical standbys sending requests should automatically become the
ones to be waited for confirmation on the primary.

thanks
Shveta

Re: Synchronizing slots from primary to standby

[shveta malik]

On Fri, Sep 22, 2023 at 6:01 PM Drouvot, Bertrand
 wrote:
>
> Hi,
>
> Thanks for all the work that has been done on this feature, and sorry
> to have been quiet on it for so long.

Thanks for looking into this.

>
> On 9/18/23 12:22 PM, shveta malik wrote:
> > On Wed, Sep 13, 2023 at 4:48 PM Hayato Kuroda (Fujitsu)
> >  wrote:
> >> Right, but I wanted to know why it is needed. One motivation seemed to 
> >> know the
> >> WAL location of physical standby, but I thought that struct WalSnd.apply 
> >> could
> >> be also used. Is it bad to assume that the physical walsender always 
> >> exists?
> >>
> >
> > We do not plan to target this case where physical slot is not created
> > between primary and physical-standby in the first draft.  In such a
> > case, slot-synchronization will be skipped for the time being. We can
> > extend this functionality (if needed) later.
> >
>
> I do think it's needed to extend this functionality. Having physical slot
> created sounds like a (too?) strong requirement as:
>
> - It has not been a requirement for Logical decoding on standby so that could 
> sounds weird
> to require it for sync slot (while it's not allowed to logical decode from 
> sync slots)
>
> - One could want to limit the WAL space used on the primary
>
> It seems that the "skipping sync as primary_slot_name not set." warning 
> message is emitted
> every 10ms, that seems too verbose to me.
>

You are right, the warning msg is way too frequent. I will optimize it
in the next version.

thanks
Shveta

Re: Remove MSVC scripts from the tree

[Michael Paquier]

On Fri, Sep 22, 2023 at 08:06:57AM +0200, Peter Eisentraut wrote:
> First we need to fix things so that we can build using meson from a
> distribution tarball, which is the subject of
> .

Thanks, missed this one.
--
Michael


signature.asc
Description: PGP signature

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Michael Paquier]

On Mon, Sep 25, 2023 at 11:27:27AM +0900, Ryoga Yoshida wrote:
> Thank you for the review. Certainly, adding a comments is a good idea. I
> added a comment.

Hmm.  How about the attached version with some tweaks?
--
Michael
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index bcaed14d02..82feb792cf 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -38,13 +38,18 @@ static WalUsage prevWalUsage;
  *
  * Must be called by processes that generate WAL, that do not call
  * pgstat_report_stat(), like walwriter.
+ *
+ * force set to true ensures that the statistics are flushed; note that
+ * this needs to acquire the pgstat shmem LWLock, waiting on it.  When
+ * set to false, the statistics may not be flushed if the lock could not
+ * be acquired.
  */
 void
 pgstat_report_wal(bool force)
 {
-	pgstat_flush_wal(force);
-
-	pgstat_flush_io(force);
+	/* like in pgstat.c, don't wait for lock acquisition when !force */
+	pgstat_flush_wal(!force);
+	pgstat_flush_io(!force);
 }
 
 /*


signature.asc
Description: PGP signature

Re: Report planning memory in EXPLAIN ANALYZE

[Andy Fan]

Hi Ashutosh,

On Fri, Sep 22, 2023 at 5:56 PM Ashutosh Bapat 
wrote:

> Hi Andy,
> Thanks for your feedback.
>
> On Fri, Sep 22, 2023 at 8:22 AM Andy Fan  wrote:
> >
> > 1). The commit message of patch 1 just says how it does but doesn't
> > say why it does. After reading through the discussion, I suggest making
> > it clearer to others.
> >
> > ...
> > After the planning is done, it may still occupy lots of memory which is
> > allocated but not pfree-d. All of these memories can't be used in the
> later
> > stage. This patch will report such memory usage in order to making some
> > future mistakes can be found in an easier way.
> > ...
>
> That's a good summary of how the memory report can be used. Will
> include a line about usage in the commit message.
>
> >
> > Planning Memory: used=15088 bytes allocated=16384 bytes
> >
> > Word 'Planning' is kind of a time duration, so the 'used' may be the
> > 'used' during the duration or 'used' after the duration. Obviously you
> > means the later one but it is not surprising others think it in the other
> > way. So I'd like to reword the metrics to:
>
> We report "PLanning Time" hence used "Planning memory". Would
> "Planner" be good instead of "Planning"?
>

I agree "Planner" is better than "Planning" personally.

>
> > Memory Occupied (Now): Parser: 1k Planner: 10k
> >
> > 'Now' is a cleaner signal that is a time point rather than a time
> > duration. 'Parser' and 'Planner' also show a timeline about the
> > important time point. At the same time, it cost very little coding
> > effort based on patch 01. Different people may have different
> > feelings about these words, I just hope I describe the goal clearly.
>
> Parsing happens before planning and that memory is not measured by
> this patch. May be separately but it's out of scope of this work.
> "used" and "allocated" are MemoryContext terms indicating memory
> actually used vs memory allocated.


Yes, I understand the terms come from MemoryContext, the complex
thing here is between time duration vs time point case. Since English
is not my native language, so I'm not too confident about insisting on
this.
So I think we can keep it as it is.


>
>
> > Personally I am pretty like patch 1, we just need to refactor some words
> > to make everything clearer.
>
> Thanks.
>

David challenged something at the begining,  but IIUC he also agree the
value of patch 01 as the previous statement after discussion. Since the
patch
is mild itself, so I will mark this commitfest entry as "Ready for
committer".
Please  correct me if anything is wrong.

-- 
Best Regards
Andy Fan

Re: Confused about gram.y referencs in Makefile?

[Japin Li]

On Mon, 25 Sep 2023 at 11:17, Tom Lane  wrote:
> Japin Li  writes:
>> I find src/backend/utils/mb/Unicode/Makefile has the following comments:
>
>>> # Note that while each script call produces two output files, to be
>>> # parallel-make safe we need to split this into two rules.  (See for
>>> # example gram.y for more explanation.)
>
>> I could not find the explanation in gram.y easily.  Would someone point
>> it out for me?  Thanks in advance!
>
> It's referring to this bit in src/backend/parser/Makefile:
>
> -
> # There is no correct way to write a rule that generates two files.
> # Rules with two targets don't have that meaning, they are merely
> # shorthand for two otherwise separate rules.  If we have an action
> # that in fact generates two or more files, we must choose one of them
> # as primary and show it as the action's output, then make all of the
> # other output files dependent on the primary, like this.  Furthermore,
> # the "touch" action is essential, because it ensures that gram.h is
> # marked as newer than (or at least no older than) gram.c.  Without that,
> # make is likely to try to rebuild gram.h in subsequent runs, which causes
> # failures in VPATH builds from tarballs.
>
> gram.h: gram.c
>   touch $@
>
> gram.c: BISONFLAGS += -d
> gram.c: BISON_CHECK_CMD = $(PERL) $(srcdir)/check_keywords.pl $< 
> $(top_srcdir)/src/include/parser/kwlist.h
> -
>
> This is indeed kind of confusing, because there's no explicit
> reference to gram.y here --- the last two lines just tweak
> the behavior of the default .y to .c rule.
>
> Maybe we should adjust the comment in Unicode/Makefile, but
> I'm not sure what would be a better reference.
>
>   regards, tom lane

Thank you!

Maybe be reference src/backend/parser/Makefile is better than current.

How about "See gram.h target's comment in src/backend/parser/Makefile"
or just "See src/backend/parser/Makefile"?

-- 
Regrads,
Japin Li

Re: Confused about gram.y referencs in Makefile?

[Tom Lane]

Japin Li  writes:
> I find src/backend/utils/mb/Unicode/Makefile has the following comments:

>> # Note that while each script call produces two output files, to be
>> # parallel-make safe we need to split this into two rules.  (See for
>> # example gram.y for more explanation.)

> I could not find the explanation in gram.y easily.  Would someone point
> it out for me?  Thanks in advance!

It's referring to this bit in src/backend/parser/Makefile:

-
# There is no correct way to write a rule that generates two files.
# Rules with two targets don't have that meaning, they are merely
# shorthand for two otherwise separate rules.  If we have an action
# that in fact generates two or more files, we must choose one of them
# as primary and show it as the action's output, then make all of the
# other output files dependent on the primary, like this.  Furthermore,
# the "touch" action is essential, because it ensures that gram.h is
# marked as newer than (or at least no older than) gram.c.  Without that,
# make is likely to try to rebuild gram.h in subsequent runs, which causes
# failures in VPATH builds from tarballs.

gram.h: gram.c
touch $@

gram.c: BISONFLAGS += -d
gram.c: BISON_CHECK_CMD = $(PERL) $(srcdir)/check_keywords.pl $< 
$(top_srcdir)/src/include/parser/kwlist.h
-

This is indeed kind of confusing, because there's no explicit
reference to gram.y here --- the last two lines just tweak
the behavior of the default .y to .c rule.

Maybe we should adjust the comment in Unicode/Makefile, but
I'm not sure what would be a better reference.

regards, tom lane

Confused about gram.y referencs in Makefile?

[Japin Li]

Hi, hackers

I find src/backend/utils/mb/Unicode/Makefile has the following comments:

> # Note that while each script call produces two output files, to be
> # parallel-make safe we need to split this into two rules.  (See for
> # example gram.y for more explanation.)
> #

I could not find the explanation in gram.y easily.  Would someone point
it out for me?  Thanks in advance!

-- 
Regrads,
Japin Li

Confused about gram.y referencs in Makefile?

[Japin Li]

Hi, hackers

I find src/backend/utils/mb/Unicode/Makefile has the following comments:

> # Note that while each script call produces two output files, to be
> # parallel-make safe we need to split this into two rules.  (See for
> # example gram.y for more explanation.)
> #

I could not find the explanation in gram.y easily.  Would someone point
it out for me?  Thanks in advance!

-- 
Regrads,
Japin Li

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Ryoga Yoshida]

On 2023-09-25 09:56, Michael Paquier wrote:

It seems to me that you are right here.  It would make sense to me to
say that force=true is equivalent to nowait=false, as in "I'm OK to
wait on the lockas I want to make sure that the stats are flushed at
this point".  Currently force=true means nowait=true, as in "I'm OK to
not have the stats flushed if I cannot take the lock".

Seeing the three callers of pgstat_report_wal(), the checkpointer
wants to force its way twice, and the WAL writer does not care if they
are not flushed immediately at it loops forever in this path.

A comment at the top of pgstat_report_wal() would be nice to document
that a bit better, at least.


Thank you for the review. Certainly, adding a comments is a good idea. I 
added a comment.


Ryoga Yoshidadiff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index bcaed14d02..6f3bac5d0f 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -38,13 +38,18 @@ static WalUsage prevWalUsage;
  *
  * Must be called by processes that generate WAL, that do not call
  * pgstat_report_stat(), like walwriter.
+ *
+ * force=false is the same as nowait=true, and the statistics will
+ * not be flushed if the lock cannot be acquired.
+ * force=true is the same as nowait=false, and waits until the lock
+ * is acquired and ensures that the statistics are flushed.
  */
 void
 pgstat_report_wal(bool force)
 {
-	pgstat_flush_wal(force);
+	pgstat_flush_wal(!force);
 
-	pgstat_flush_io(force);
+	pgstat_flush_io(!force);
 }
 
 /*

Re: Synchronizing slots from primary to standby

[Peter Smith]

FYI -- v19 failed to apply cleanly with the latest HEAD.

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v19-0001-Allow-logical-walsenders-to-wait-for-physical-st.patch
error: patch failed: src/test/recovery/meson.build:44
error: src/test/recovery/meson.build: patch does not apply

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: pg_upgrade and logical replication

[Michael Paquier]

On Wed, Sep 20, 2023 at 09:38:56AM +0900, Michael Paquier wrote:
> And this code path is used to start postmaster instances for old and
> new clusters.  So it seems to me that it is incorrect if this is not
> conditional based on the cluster version.

Avoiding the startup of bgworkers during pg_upgrade is something that
worries me a bit, actually, as it could be useful in some cases like
monitoring?  That would be fancy, for sure..  For now and seeing a
lack of consensus on this larger matter, I'd like to propose a check
for IsBinaryUpgrade into ApplyLauncherRegister() instead as it makes
no real sense to start apply workers in this context.  That would be
equivalent to max_logical_replication_workers = 0.

Amit, Vignesh, would the attached be OK for both of you?

(Vignesh has posted a slightly different version of this patch on a
different thread, but the subscriber part should be part of this
thread with the subscribers, I assume.) 
--
Michael
From 2df408695163eb46bfe7efa9a9ccc07ff5fab183 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 25 Sep 2023 10:54:59 +0900
Subject: [PATCH] Prevent startup of logical replication launcher in binary
 upgrade mode

The logical replication launcher may start apply workers during an
upgrade, which could be the cause of corruptions on a new cluster if
these are able to apply changes before the physical files are copied
over.

The chance of being able to do so should be small as pg_upgrade uses its
own port and unix domain directory (customizable as well with
--socketdir), but just preventing the launcher to start is safer at the
end, because we are then sure that no changes would ever be applied.

Author: Vignesh C
Discussion: https://postgr.es/m/CALDaNm2g9ZKf=y8X6z6MsLCuh8WwU-=q6plj35nfi2m5bzn...@mail.gmail.com
---
 src/backend/replication/logical/launcher.c | 9 +
 src/bin/pg_upgrade/server.c| 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 7882fc91ce..9c610edbeb 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -925,6 +925,15 @@ ApplyLauncherRegister(void)
 {
 	BackgroundWorker bgw;
 
+	/*
+	 * We don't want the launcher to run in binary upgrade mode because it may
+	 * start apply workers which could start receiving changes from the
+	 * publisher before the physical files are put in place, causing
+	 * corruption on the new cluster upgrading to.
+	 */
+	if (IsBinaryUpgrade)
+		return;
+
 	if (max_logical_replication_workers == 0)
 		return;
 
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 0bc3d2806b..edbc101269 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -228,7 +228,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 #endif
 
 	/*
-	 * Use -b to disable autovacuum.
+	 * Use -b to disable autovacuum and logical replication launcher.
 	 *
 	 * Turn off durability requirements to improve object creation speed, and
 	 * we only modify the new cluster, so only use it there.  If there is a
-- 
2.40.1



signature.asc
Description: PGP signature

Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly

[Michael Paquier]

On Fri, Sep 22, 2023 at 01:58:37PM +0900, Ryoga Yoshida wrote:
> pgstat_report_wal() calls pgstat_flush_wal() and pgstat_flush_io(). When
> calling them, pgstat_report_wal() specifies its argument "force" as the
> argument of them, as follows. But according to the code of
> pgstat_flush_wal() and pgstat_flush_io(), their argument is "nowait" and its
> meaning seems the opposite of "force". This means that, even when
> checkpointer etc calls pgstat_report_wal() with force=true to forcibly flush
> the statistics, pgstat_flush_wal() and pgstat_flush_io() skip flushing the
> statistics if they fail to acquire the lock immediately because they are
> called with nowait=true. This seems unexpected behavior and a bug.

It seems to me that you are right here.  It would make sense to me to
say that force=true is equivalent to nowait=false, as in "I'm OK to
wait on the lockas I want to make sure that the stats are flushed at
this point".  Currently force=true means nowait=true, as in "I'm OK to
not have the stats flushed if I cannot take the lock".

Seeing the three callers of pgstat_report_wal(), the checkpointer
wants to force its way twice, and the WAL writer does not care if they
are not flushed immediately at it loops forever in this path.

A comment at the top of pgstat_report_wal() would be nice to document
that a bit better, at least.
--
Michael


signature.asc
Description: PGP signature

Re: DROP DATABASE is interruptible

[Peter Eisentraut]

I noticed that this patch set introduced this pg_dump test:

On 12.07.23 03:59, Andres Freund wrote:

+   'CREATE DATABASE invalid...' => {
+   create_order => 1,
+   create_sql => q(CREATE DATABASE invalid; UPDATE pg_database SET 
datconnlimit = -2 WHERE datname = 'invalid'),
+   regexp => qr/^CREATE DATABASE invalid/m,
+   not_like => {
+   pg_dumpall_dbprivs => 1,
+   },
+   },


But the key "not_like" isn't used for anything by that test suite. 
Maybe "unlike" was meant?  But even then it would be useless because the 
"like" key is empty, so there is nothing that "unlike" can subtract 
from.  Was there something expected from the mention of 
"pg_dumpall_dbprivs"?


Perhaps it would be better to write out

like => {},

explicitly, with a comment, like some other tests are doing.

Re: Fix bug in VACUUM and ANALYZE docs

[Michael Paquier]

On Sun, Sep 24, 2023 at 06:30:32PM -0500, Karl O. Pinc wrote:
> I signed up to review, but I think that perhaps commitfest
> https://commitfest.postgresql.org/45/4574/
> needs marking as applied and done?

Indeed.  I did not notice that there was a CF entry for this one.
Closed it now.
--
Michael


signature.asc
Description: PGP signature

Re: GUC for temporarily disabling event triggers

[Michael Paquier]

On Fri, Sep 22, 2023 at 05:29:01PM +0200, Daniel Gustafsson wrote:
> Since the main driver for this is to reduce the usage/need for single-user 
> mode
> I also reworded the patch slightly.  Instead of phrasing this as an 
> alternative
> to single-user mode, I reversed it such that single-user mode is an 
> alternative
> to this GUC.

Okay.

+   be disabled by setting it to false. Setting the value
+   to true will not disable any event triggers, this

This uses a double negation.  Perhaps just "Setting this value to true
allows all event triggers to run."

003_check_guc.pl has detected a failure because event_triggers is
missing in postgresql.conf.sample while it is not marked with
GUC_NOT_IN_SAMPLE anymore.

Keeping the docs consistent with the sample file, I would suggest the
attached on top of your v9.
--
Michael
From 2ad4989a71c40b7d6de8fcb095b057377cd05bf5 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson 
Date: Fri, 22 Sep 2023 16:39:07 +0200
Subject: [PATCH v9 1/2] Add GUC for temporarily disabling event triggers

In order to troubleshoot misbehaving or buggy event triggers, the
documented advice is to enter single-user mode.  In an attempt to
reduce the number of situations where single-user mode is required
(or even recommended) for non-extraordinary maintenance, this GUC
allows to temporarily suspend event triggers.

This was originally extracted from a larger patchset which aimed
at supporting event triggers on login events.

Reviewed-by: Ted Yu 
Reviewed-by: Mikhail Gribkov 
Reviewed-by: Justin Pryzby 
Reviewed-by: Michael Paquier 
Discussion: https://postgr.es/m/9140106e-f9bf-4d85-8fc8-f2d3c094a...@yesql.se
---
 src/include/commands/event_trigger.h|  2 ++
 src/backend/commands/event_trigger.c| 20 +++--
 src/backend/utils/misc/guc_tables.c | 11 ++
 src/test/regress/expected/event_trigger.out | 22 +++
 src/test/regress/sql/event_trigger.sql  | 24 +
 doc/src/sgml/config.sgml| 19 
 doc/src/sgml/ref/create_event_trigger.sgml  |  9 +---
 7 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index 5ed6ece555..1c925dbf25 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -29,6 +29,8 @@ typedef struct EventTriggerData
 	CommandTag	tag;
 } EventTriggerData;
 
+extern PGDLLIMPORT bool event_triggers;
+
 #define AT_REWRITE_ALTER_PERSISTENCE	0x01
 #define AT_REWRITE_DEFAULT_VAL			0x02
 #define AT_REWRITE_COLUMN_REWRITE		0x04
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index d4b00d1a82..bd812e42d9 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -72,6 +72,9 @@ typedef struct EventTriggerQueryState
 
 static EventTriggerQueryState *currentEventTriggerState = NULL;
 
+/* GUC parameter */
+bool		event_triggers = true;
+
 /* Support for dropped objects */
 typedef struct SQLDropObject
 {
@@ -657,8 +660,11 @@ EventTriggerDDLCommandStart(Node *parsetree)
 	 * wherein event triggers are disabled.  (Or we could implement
 	 * heapscan-and-sort logic for that case, but having disaster recovery
 	 * scenarios depend on code that's otherwise untested isn't appetizing.)
+	 *
+	 * Additionally, event triggers can be disabled with a superuser-only GUC
+	 * to make fixing database easier as per 1 above.
 	 */
-	if (!IsUnderPostmaster)
+	if (!IsUnderPostmaster || !event_triggers)
 		return;
 
 	runlist = EventTriggerCommonSetup(parsetree,
@@ -692,9 +698,9 @@ EventTriggerDDLCommandEnd(Node *parsetree)
 
 	/*
 	 * See EventTriggerDDLCommandStart for a discussion about why event
-	 * triggers are disabled in single user mode.
+	 * triggers are disabled in single user mode or via GUC.
 	 */
-	if (!IsUnderPostmaster)
+	if (!IsUnderPostmaster || !event_triggers)
 		return;
 
 	/*
@@ -740,9 +746,9 @@ EventTriggerSQLDrop(Node *parsetree)
 
 	/*
 	 * See EventTriggerDDLCommandStart for a discussion about why event
-	 * triggers are disabled in single user mode.
+	 * triggers are disabled in single user mode or via a GUC.
 	 */
-	if (!IsUnderPostmaster)
+	if (!IsUnderPostmaster || !event_triggers)
 		return;
 
 	/*
@@ -811,9 +817,9 @@ EventTriggerTableRewrite(Node *parsetree, Oid tableOid, int reason)
 
 	/*
 	 * See EventTriggerDDLCommandStart for a discussion about why event
-	 * triggers are disabled in single user mode.
+	 * triggers are disabled in single user mode or via a GUC.
 	 */
-	if (!IsUnderPostmaster)
+	if (!IsUnderPostmaster || !event_triggers)
 		return;
 
 	/*
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index bdb26e2b77..16ec6c5ef0 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -37,6 +37,7 @@
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/asyn

Re: Fix bug in VACUUM and ANALYZE docs

[Karl O. Pinc]

On Wed, 20 Sep 2023 13:39:02 +0900
Michael Paquier  wrote:

> On Wed, Sep 20, 2023 at 09:43:15AM +0900, Shinya Kato wrote:

> > You're right. It looks good to me.  
> 
> Right, it feels like there has been a lot of copy-paste in this area
> of the docs.  Applied down to 16.

I signed up to review, but I think that perhaps commitfest
https://commitfest.postgresql.org/45/4574/
needs marking as applied and done?

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein

Various small doc improvements; plpgsql, schemas, permissions, oidvector

[Karl O. Pinc]

Hi,

I have made various, mostly unrelated to each other,
small improvements to the documentation.  These
are usually in the areas of plpgsql, schemas, and permissions.
Most change 1 lines, but some supply short overviews.

"Short" is subjective, so if these need to be
broken into different threads or different
commitfest entries let me know.  I'm starting
simple and submitting a single patch.

Attached: various_doc_patches_v1.patch

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6bc1b215db..ea70dd3597 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10,7 +10,27 @@
 
   
There are many configuration parameters that affect the behavior of
-   the database system. In the first section of this chapter we
+   the database system.
+   A single master list of all parameters and their characteristics is not
+   provided in the documentation.
+   The pg_settings view into the system catalogs can provide such summaries,
+   e.g:
+  
+
+
+-- Describe all configuration parameters.
+SELECT name, short_desc FROM pg_settings ORDER BY name;
+
+-- Show the current configuration of the connected database and session.
+SELECT name, setting, unit FROM pg_settings ORDER BY name;
+
+-- Show the means available to change the setting; whether the setting is
+-- per-cluster, per-database, per-session, etc.
+SELECT name, context FROM pg_settings ORDER BY name;
+
+  
+  
+   In the first section of this chapter we
describe how to interact with configuration parameters. The subsequent sections
discuss each parameter in detail.
   
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 8d32a8c9c5..91cc1e5cb0 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -4754,6 +4754,17 @@ INSERT INTO mytable VALUES(-1);  -- fails
 signed-versus-unsigned confusion if you do this.)

 
+   
+oidvector
+   
+
+   
+ The legacy oidvector type can be cast to an array of OIDs,
+ and vice versa, for examination and manipulation.
+ The resultant array of OIDs, unlike a typical array, is indexed
+ zero-relative.
+   
+

 The OID alias types have no operations of their own except
 for specialized input and output routines.  These routines are able
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..df8a373652 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3106,12 +3106,12 @@ SHOW search_path;

 The first schema in the search path that exists is the default
 location for creating new objects.  That is the reason that by
-default objects are created in the public schema.  When objects
-are referenced in any other context without schema qualification
-(table modification, data modification, or query commands) the
-search path is traversed until a matching object is found.
-Therefore, in the default configuration, any unqualified access
-again can only refer to the public schema.
+default objects are created in the public schema.
+When objects are referenced in a context without schema qualification
+(table modification, data modification, or query commands) the search path
+is traversed until a matching object is found.
+Therefore, again, in the default configuration, any unqualified access
+refers only to objects in the public schema.

 

@@ -3162,6 +3162,53 @@ SELECT 3 OPERATOR(pg_catalog.+) 4;

   
 
+  
+Schema Resolution
+
+
+ schema
+ resolution
+
+
+
+  Schema resolution happens when SQL is run.
+
+  Exactly what this means is usually obvious; using an unqualified table
+  name when creating a table creates the table in the first schema of the
+  search path in effect, the current search path is used to find
+  unqualified table names when executing a SELECT, and so forth.
+  But there are less obvious implications when it comes to statements
+  that manipulate database objects:
+  tables, triggers, functions and the like.
+
+
+
+  Most SQL expressions appearing within the statements that manipulate
+  database objects are resolved at the time of manipulation.
+  Consider the creation of tables and triggers.
+  The schemas of foreign key table references, the functions and operators
+  used in table and column constraint expressions, table partition
+  expressions, and so forth are resolved at the time of table creation.
+  So is the schema of the function named when a trigger is created.
+  These already-resolved tables, functions, operators,
+  etc. need not be in the search path when the
+  constraints or triggers are executed, when the content of the table is
+  modified and the data validation occurs.
+  This is just as if all the objects were fully schema qualified in th

Re: Invalidate the subscription worker in cases where a user loses their superuser status

[vignesh C]

On Sat, 23 Sept 2023 at 11:28, Amit Kapila  wrote:
>
> On Sat, Sep 23, 2023 at 1:27 AM vignesh C  wrote:
> >
> >
> > Fixed this issue by checking if the subscription owner has changed
> > from superuser to non-superuser in case the pg_authid rows changes.
> > The attached patch has the changes for the same.
> >
>
> @@ -3952,7 +3953,9 @@ maybe_reread_subscription(void)
>   newsub->passwordrequired != MySubscription->passwordrequired ||
>   strcmp(newsub->origin, MySubscription->origin) != 0 ||
>   newsub->owner != MySubscription->owner ||
> - !equal(newsub->publications, MySubscription->publications))
> + !equal(newsub->publications, MySubscription->publications) ||
> + (!superuser_arg(MySubscription->owner) &&
> + MySubscription->isownersuperuser))
>   {
>   if (am_parallel_apply_worker())
>   ereport(LOG,
> @@ -4605,6 +4608,13 @@ InitializeLogRepWorker(void)
>   proc_exit(0);
>   }
>
> + /*
> + * Fetch subscription owner is a superuser. This value will be later
> + * checked to see when there is any change with this role and the worker
> + * will be restarted if required.
> + */
> + MySubscription->isownersuperuser = superuser_arg(MySubscription->owner);
>
> Why didn't you filled this parameter in GetSubscription() like other
> parameters? If we do that then the comparison of first change in your
> patch will look similar to all other comparisons.

I felt this variable need not be added to the pg_subscription catalog
table, instead we could save the state of subscription owner when the
worker is started and compare this value during invalidations. As this
information is added only to the memory Subscription structure and not
added to the catalog FormData_pg_subscription, the checking is
slightly different in this case. Also since this variable will be used
only within the worker, I felt we need not add it to the catalog.

Regards,
Vignesh

Re: Eager page freeze criteria clarification

[Melanie Plageman]

On Sat, Sep 23, 2023 at 3:53 PM Melanie Plageman
 wrote:
>
> Workload F:
>
> +--++-++--+
> | algo | WAL GB | cptr bgwriter writes| other reads/writes | IO time AV 
> worker|
> +--++-+-+-+
> |M |173 |   1,202,231 | 53,957,448 |   12,389 
> |
> |4 |189 |   1,212,521 | 55,589,140 |   13,084 
> |
> |5 |173 |   1,194,242 | 54,260,118 |   13,407 
> |
> +--++-++--+
>
> +--+--+
> | algo | P99 latency  |
> +--+--+
> |M |   19875  |
> |4 |   19314  |
> |5 |   19701  |
> +--+--+

Andres mentioned that the P99 latency for the COPY workload (workload F)
might not be meaningful, so I have calculated the duration total, mean,
median, min, max and standard deviation in milliseconds.

Workload F:
+--++---++++-+
| algo |  Total |   Mean| Median |Min |Max |  Stddev |
+--++---++++-+
|M |  1,270,903 | 18,155| 17,755 | 17,090 | 19,994 | 869 |
|4 |  1,167,135 | 16,673| 16,421 | 15,585 | 19,485 | 811 |
|5 |  1,250,145 | 17,859| 17,704 | 15,763 | 19,871 |   1,009 |
+--++---++++-+

Interestingly, algorithm 4 had the lowest total duration for all COPYs.
Some investigation of other data collected during the runs led us to
believe this may be due to autovacuum workers doing more IO with
algorithm 4 and thus generating more WAL and ending up initializing more
WAL files themselves. Whereas on master and with algorithm 5, client
backends had to initialize WAL files themselves, leading COPYs to take
longer. This was supported by the presence of more WALInit wait events
for client backends on master and with algorithm 5.

Calculating these made me realize that my conclusions about the work
queue workload (workload I) didn't make much sense. Because this
workload updated a non-indexed column, most pruning was HOT pruning done
on access and basically no page freezing was done by vacuum. This means
we weren't seeing negative performance effects of freezing related to
the work queue table.

The difference in this benchmark came from the relatively poor
performance of the concurrent COPYs when that table was frozen more
aggressively. I plan to run a new version of this workload which updates
an indexed column for comparison and does not use a concurrent COPY.

This is the duration total, mean, median, min, max, and standard
deviation in milliseconds of the COPYs which ran concurrently with the
work queue pgbench.

Workload I COPYs:
+--++---++++-+
| algo |  Total |  Mean | Median |   Min  |   Max  |  Stddev |
+--++---++++-+
|M | 191,032|  4,898|  4,726 |  4,486 |  9,353 | 800 |
|4 | 193,534|  4,962|  4,793 |  4,533 |  9,381 | 812 |
|5 | 194,351|  4,983|  4,771 |  4,617 |  9,159 | 783 |
+--++---++++-+

I think this shows that algorithm 4 COPYs performed the worst. This is
in contrast to the COPY-only workload (F) which did not show worse
performance for algorithm 4. I think this means I should modify the work
queue example and use something other than concurrent COPYs to avoid
obscuring characteristics of the work queue example.

- Melanie

Re: How to Know the number of attrs?

[Erik Wienhold]

On 2023-09-24 20:56 +0800, jacktby jacktby wrote:
> typedef struct TupleDescData
> {
>   int natts;  /* number of attributes 
> in the tuple */
>   Oid tdtypeid;   /* composite type ID 
> for tuple type */
>   int32   tdtypmod;   /* typmod for tuple type */
>   int tdrefcount; /* reference count, or 
> -1 if not counting */
>   TupleConstr *constr;/* constraints, or NULL if none */
>   /* attrs[N] is the description of Attribute Number N+1 */
>   FormData_pg_attribute attrs[FLEXIBLE_ARRAY_MEMBER];
> } TupleDescData;
> 
> Hi, the attrs use FLEXIBLE_ARRAY_MEMBER, so which API should I use to
> get the real length of this array?

Use the natts field of TupleDescData.

-- 
Erik

How to Know the number of attrs?

[jacktby jacktby]

typedef struct TupleDescData
{
int natts;  /* number of attributes 
in the tuple */
Oid tdtypeid;   /* composite type ID 
for tuple type */
int32   tdtypmod;   /* typmod for tuple type */
int tdrefcount; /* reference count, or 
-1 if not counting */
TupleConstr *constr;/* constraints, or NULL if none */
/* attrs[N] is the description of Attribute Number N+1 */
FormData_pg_attribute attrs[FLEXIBLE_ARRAY_MEMBER];
}   TupleDescData;

Hi, the attrs use FLEXIBLE_ARRAY_MEMBER, so which API should I use to get the 
real length of this array?

How to Know the number of attrs?

[jacktby jacktby]

typedef struct TupleDescData
{
int natts;  /* number of attributes 
in the tuple */
Oid tdtypeid;   /* composite type ID 
for tuple type */
int32   tdtypmod;   /* typmod for tuple type */
int tdrefcount; /* reference count, or 
-1 if not counting */
TupleConstr *constr;/* constraints, or NULL if none */
/* attrs[N] is the description of Attribute Number N+1 */
FormData_pg_attribute attrs[FLEXIBLE_ARRAY_MEMBER];
}   TupleDescData;

Hi, the attrs use FLEXIBLE_ARRAY_MEMBER, so which API should I use to get the 
real length of this array?

Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

LGTM too!

>> a bit to make the diff smaller,
I couldn't think from that perspective. Thanks for your update, Daniel-san.

Masaki Kuwamura