Add a pg_get_query_def function (was Re: Deparsing rewritten query)

[Julien Rouhaud]

On Fri, Mar 25, 2022 at 05:49:04PM -0400, Tom Lane wrote:
> Julien Rouhaud  writes:
> > I'm attaching the correct patch this time, sorry about that.
> 
> While I'm okay with this in principle, as it stands it fails
> headerscheck/cpluspluscheck:
> 
> $ src/tools/pginclude/headerscheck 
> In file included from /tmp/headerscheck.Oi8jj3/test.c:2:
> ./src/include/utils/ruleutils.h:42:35: error: unknown type name 'StringInfo'; 
> did you mean 'String'?
>  void  get_query_def(Query *query, StringInfo buf, List *parentnamespace,
>^~
>String
> ./src/include/utils/ruleutils.h:43:9: error: unknown type name 'TupleDesc'
>  TupleDesc resultDesc,
>  ^

Ah thanks for the info.  I actually never tried headerscheck/cplupluscheck
before.

> We could of course add the necessary #include's to ruleutils.h,
> but considering that we seem to have been at some pains to minimize
> its #include footprint, I'm not really happy with that approach.
> I'm inclined to think that maybe a wrapper function with a slightly
> simplified interface would be a better way to go.  The deparsed string
> could just be returned as a palloc'd "char *", unless you have some reason
> to need it to be in a StringInfo.  I wonder which of the other parameters
> really need to be exposed, too.  Several of them seem to be more internal
> to ruleutils.c than something that outside callers would care to
> manipulate.  For instance, since struct deparse_namespace isn't exposed,
> there really isn't any way to pass anything except NIL for
> parentnamespace.
> 
> The bigger picture here is that get_query_def's API has changed over time
> internally to ruleutils.c, and I kind of expect that that might continue
> in future, so having a wrapper function with a more stable API could be a
> good thing.

Fair point.  That's a much better approach and goes well with the rest of the
exposed functions in that file.  I went with a pg_get_querydef, getting rid of
the StringInfo and the List and using the same "bool pretty" flag as used
elsewhere.  While doing so, I saw that there were a lot of copy/pasted code for
the pretty flags, so I added a GET_PRETTY_FLAGS(pretty) macro to avoid adding
yet another occurrence.  I also kept the wrapColument and startIdent as they
can be easily used by callers.
>From ca49e5e96fcab75c8e19a42d701b16ac96ee9d43 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Tue, 29 Jun 2021 00:07:04 +0800
Subject: [PATCH v4] Add a pg_get_query_def() wrapper around get_query_def().

This function can be useful for external modules, for instance if they want to
display a statement after the rewrite stage.

In order to emit valid SQL, make sure that any subquery RTE comes with an
alias.  This is always the case for user facing queries, as the parser will
refuse a subquery without an alias, but that may not be the case for a Query
after rewriting for instance.

Catversion is bumped.

Author: Julien Rouhaud
Reviewed-by: Pavel Stehule
Discussion: https://postgr.es/m/20210627041138.zklczwmu3ms4ufnk@nol
---
 src/backend/utils/adt/ruleutils.c | 49 ---
 src/include/utils/ruleutils.h |  2 ++
 2 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index 7f4f3f7369..c9a1dde65f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -90,6 +90,8 @@
 #define PRETTYFLAG_INDENT  0x0002
 #define PRETTYFLAG_SCHEMA  0x0004
 
+#define GET_PRETTY_FLAGS(pretty) ((pretty) ? (PRETTYFLAG_PAREN | 
PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) : PRETTYFLAG_INDENT)
+
 /* Default line length for pretty-print wrapping: 0 means wrap always */
 #define WRAP_COLUMN_DEFAULT0
 
@@ -526,7 +528,7 @@ pg_get_ruledef_ext(PG_FUNCTION_ARGS)
int prettyFlags;
char   *res;
 
-   prettyFlags = pretty ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | 
PRETTYFLAG_SCHEMA) : PRETTYFLAG_INDENT;
+   prettyFlags = GET_PRETTY_FLAGS(pretty);
 
res = pg_get_ruledef_worker(ruleoid, prettyFlags);
 
@@ -647,7 +649,7 @@ pg_get_viewdef_ext(PG_FUNCTION_ARGS)
int prettyFlags;
char   *res;
 
-   prettyFlags = pretty ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | 
PRETTYFLAG_SCHEMA) : PRETTYFLAG_INDENT;
+   prettyFlags = GET_PRETTY_FLAGS(pretty);
 
res = pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT);
 
@@ -667,7 +669,7 @@ pg_get_viewdef_wrap(PG_FUNCTION_ARGS)
char   *res;
 
/* calling this implies we want pretty printing */
-   prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA;
+   prettyFlags = GET_PRETTY_FLAGS(true);
 
res = pg_get_viewdef_worker(viewoid, prettyFlags, wrap);
 
@@ -713,7 +715,7 @@ pg_get_viewdef_name_ext(PG_FUNCTION_ARGS)
Oid

Re: make MaxBackends available in _PG_init

[Julien Rouhaud]

On Sat, Mar 26, 2022 at 10:23:16AM -0700, Andres Freund wrote:
>
> On 2022-03-26 15:22:03 +0800, Julien Rouhaud wrote:
> > On Fri, Mar 25, 2022 at 03:23:17PM -0700, Andres Freund wrote:
> > >
> > > I don't really understand. The issue that started this thread was bugs in
> > > extensions due to accessing MaxBackends before it is initialized - which 
> > > the
> > > patch prevents.
> > 
> > Well, the patch prevents accessing a 0-valued MaxBackends but doesn't do
> > anything to solve the original complaint.  It's not like extensions won't 
> > need
> > to access that information during _PG_init anymore.
> 
> It resolves the pretty common bug that an extension breaks once it's used via
> s_p_l instead of loaded on-demand because MaxBackends isn't initialized in the
> s_p_l case.

I can hear the argument.  However I don't know any extension that relies on
MaxBackends and doesn't need to be in s_p_l, and unless I'm missing something
no one provided such an example, only people needing the value for
RequestAddinShmemSpace().

> > And indeed, any third party code that previously needed to access what
> > MaxBackends is supposed to store should already be using that formula, and
> > the new GetMaxBackends() doesn't do anything about it.
> 
> It couldn't rely on MaxBackends before. It can't rely on GetMaxBackends()
> now. You can see why I think that what you want is unrelated to the
> introduction of GetMaxBackends().

Sure, but code also couldn't really rely on MaxConnections or any other similar
GUCs and yet nothing is done for that, and the chosen approach doesn't help for
that.

The only difference is that those GUCs are less broken, as in the value is
likely to be valid more often, and closer to the final value when it's not.
But still broken.

I think GetMaxBackends() is more likely to force authors to rely on computing
the value using the underlying GUCs, since there's nothing else that can be
done.  And if that's an acceptable answer, why aren't we computing MaxBackends
before and after processing s_p_l?

> If we introduce a separate hook that allows to influence things like
> max_connections or whatnot we'd *even more* need a way to verify whether it's
> legal to access MaxBackends in that moment.

Again, I'm not opposed to validating that MaxBackends is valid when third-party
code is accessing it, quite the opposite.  I'm opposed to do so *only* for
MaxBackends, and without providing a way for third-party code to access that
value when they need it, which is for all the cases I know (after more digging
that's now 5 extensions) for RequestAddinShmemSpace().

Re: Is there any documentation on how to correctly create extensions in HA(primary-standby) setup?

[Bharath Rupireddy]

On Fri, Mar 25, 2022 at 10:20 AM Greg Stark  wrote:
>
> This doesn't seem to be getting any further attention. It sounds like
> Julien didn't agree with the scope of the text. Bharath do you think
> Julien's comments make sense? Will you have a chance to look at this?

Thanks Greg. I was busy with other features.

Thanks Julien for the off-list discussion. I tried to address review
comments in the v3 patch attached. Now, I've added the notes in
high-availability.sgml which sort of suits more and closer to physical
replicatioin than contrib.sgml or extend.sgml.

Thoughts?

Regards,
Bharath Rupireddy.


v3-0001-Document-configuring-an-external-module-in-physic.patch
Description: Binary data

Re: pg_relation_size on partitioned table

[Japin Li]

On Sat, 26 Mar 2022 at 22:16, Bharath Rupireddy 
 wrote:
> On Sat, Mar 26, 2022 at 11:35 AM Michael Paquier  wrote:
>>
>> On Fri, Mar 25, 2022 at 08:52:40PM +0800, Japin Li wrote:
>> > Could we provide a function to get the total size of the partition table
>> > though the partitioned table name?  Maybe we can extend
>> > the pg_relation_size() to get the total size of partition tables through
>> > the partitioned table name.
>>
>> There are already many replies on this thread, but nobody has
>> mentioned pg_partition_tree() yet, so here you go.  You could use that
>> in combination with pg_relation_size() to get the whole size of a tree
>> depending on your needs.
>
> Yeah. The docs have a note on using it for finding partitioned table size:
>
>
> For example, to check the total size of the data contained in a
> partitioned table measurement, one could use the
> following query:
> 
> SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
>   FROM pg_partition_tree('measurement');
> 
>
>

Thanks for all of you!  The above code does what I want.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Document atthasmissing default optimization avoids verification table scan

[David G. Johnston]

On Sat, Mar 26, 2022 at 4:36 PM Tom Lane  wrote:

> "David G. Johnston"  writes:
> > Or, we can leave it where things are and make sure the reader understands
> > there are two paths to having a NOT NULL constraint on the newly added
> > column.  Something like:
>
> > "If you plan on having a NOT NULL constraint on the newly added column
> you
> > should add it as a column constraint during the ADD COLUMN command.  If
> you
> > add it later via ALTER COLUMN SET NOT NULL the table will have to be
> > completely scanned in order to ensure that no null values were inserted."
>
> The first way also requires having a non-null DEFAULT, of course, and
> then also that default value must be a constant (else you end up with
> a table rewrite which is even worse).  This sort of interaction
> between features is why I feel that a separate unified discussion
> is the only reasonable solution.
>
>
The paragraph it is being added to discusses the table rewrite already.
This does nothing to contradict the fact that a table rewrite might still
have to happen.

The goal of this sentence is to tell the user to make sure they don't
forget to add the NOT NULL during the column add so that they don't have to
incur a future table scan by executing ALTER COLUMN SET NOT NULL.

I am assuming that the user understands when a table rewrite has to happen
and that the presence of NOT NULL in the ADD COLUMN doesn't impact that.
And if a table rewrite happens that a table scan happens implicitly.
Admittedly, this doesn't directly address the original complaint, but by
showing how the two commands differ I believe the confusion will go away.
SET NOT NULL performs a scan, ADD COLUMN NOT NULL does not; it just might
require something worse if the supplied default is volatile.

David J.

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Sun, Mar 27, 2022 at 12:37:27AM +0100, Daniel Gustafsson wrote:
> > On 26 Mar 2022, at 17:21, Justin Pryzby  wrote:
> 
> > I suggested off-list to add an 0099 patch to change LZ4 to the default, to
> > exercise it more on CI.
> 
> No need to change the defaults in autoconf for that.  The CFBot uses the 
> cirrus
> file in the tree so changing what the job includes can be easily done 
> (assuming
> the CFBot hasn't changed this recently which I think it hasn't).  I used that
> trick in the NSS patchset to add a completely new job for --with-ssl=nss 
> beside
> the --with-ssl=openssl job.

I think you misunderstood - I'm suggesting not only to use with-lz4 (which was
always true since 93d973494), but to change pg_dump -Fc and -Fd to use LZ4 by
default (the same as I suggested for toast_compression, wal_compression, and
again in last year's patch to add zstd compression to pg_dump, for which
postgres was not ready).

@@ -781,6 +807,11 @@ main(int argc, char **argv)
compress.alg = COMPR_ALG_LIBZ;
compress.level = Z_DEFAULT_COMPRESSION;
 #endif
+
+#ifdef USE_ZSTD
+   compress.alg = COMPR_ALG_ZSTD; // Set default for 
testing purposes
+   compress.level = ZSTD_CLEVEL_DEFAULT;
+#endif

Re: Column Filtering in Logical Replication

[Tomas Vondra]

On 3/26/22 22:58, Tomas Vondra wrote:
> On 3/26/22 22:55, Tom Lane wrote:
>> Tomas Vondra  writes:
>>> On 3/26/22 22:37, Tom Lane wrote:
 This smells like an uninitialized-variable problem, but I've had
 no luck finding any problem under valgrind.  Not sure how to progress
 from here.
>>
>>> I think I see the problem - there's a CREATE SUBSCRIPTION but the test
>>> is not waiting for the tablesync to complete, so sometimes it finishes
>>> in time and sometimes not. That'd explain the flaky behavior, and it's
>>> just this one test that misses the sync AFAICS.
>>
>> Ah, that would also fit the symptoms.
>>
> 
> I'll go over the test to check if some other test misses that, and
> perhaps do a bit of testing, and then push a fix.
> 

Pushed. I checked the other tests in 031_column_list.pl and I AFAICS all
of them are waiting for the sync correctly.


[rolls eyes] I just noticed I listed the file as .sql in the commit
message. Not great.


regards

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

Re: Add LZ4 compression in pg_dump

[Daniel Gustafsson]

> On 26 Mar 2022, at 17:21, Justin Pryzby  wrote:

> I suggested off-list to add an 0099 patch to change LZ4 to the default, to
> exercise it more on CI.

No need to change the defaults in autoconf for that.  The CFBot uses the cirrus
file in the tree so changing what the job includes can be easily done (assuming
the CFBot hasn't changed this recently which I think it hasn't).  I used that
trick in the NSS patchset to add a completely new job for --with-ssl=nss beside
the --with-ssl=openssl job.

--
Daniel Gustafsson   https://vmware.com/

Re: Document atthasmissing default optimization avoids verification table scan

[Tom Lane]

"David G. Johnston"  writes:
> Or, we can leave it where things are and make sure the reader understands
> there are two paths to having a NOT NULL constraint on the newly added
> column.  Something like:

> "If you plan on having a NOT NULL constraint on the newly added column you
> should add it as a column constraint during the ADD COLUMN command.  If you
> add it later via ALTER COLUMN SET NOT NULL the table will have to be
> completely scanned in order to ensure that no null values were inserted."

The first way also requires having a non-null DEFAULT, of course, and
then also that default value must be a constant (else you end up with
a table rewrite which is even worse).  This sort of interaction
between features is why I feel that a separate unified discussion
is the only reasonable solution.

regards, tom lane

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Tom Lane]

Thomas Munro  writes:
> On Sat, Mar 26, 2022 at 4:14 PM Justin Pryzby  wrote:
>> I see it here (and in cfbot), although I'm not sure how you created a new
>> patch for the active CF, and not for the next CF.

> Anyone who has ever been a CF manager has this power, it seems.  I did
> it myself once, by accident, and got told off by the active CF
> manager.

I'm not sure what the policy is for that.  I have done it myself,
although I've never been a CF manager, so maybe it was granted
to all committers?

regards, tom lane

Re: Document atthasmissing default optimization avoids verification table scan

[David G. Johnston]

On Sat, Mar 26, 2022 at 4:14 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

>
> I would suggest rewriting 0001 to target ALTER COLUMN instead of in the
> generic notes section (in the paragraph beginning "Adding a column with a
> volatile DEFAULT") for the desired clarification.
>
>
Or, we can leave it where things are and make sure the reader understands
there are two paths to having a NOT NULL constraint on the newly added
column.  Something like:

"If you plan on having a NOT NULL constraint on the newly added column you
should add it as a column constraint during the ADD COLUMN command.  If you
add it later via ALTER COLUMN SET NOT NULL the table will have to be
completely scanned in order to ensure that no null values were inserted."

David J.

Re: Small TAP tests cleanup for Windows and unused modules

[Daniel Gustafsson]

> On 25 Mar 2022, at 13:42, Dagfinn Ilmari Mannsåker  wrote:

> A quick git grep¹ revealed a few more extraneous `use Config;`
> statements (and manual inspection a few more unused modules in one
> file).

Thanks for digging these up, they look correct to me.

> src/bin/pg_ctl/t/001_start_stop.pl | 3 ---
> src/bin/pg_rewind/t/RewindTest.pm  | 1 -

These Config references were removed in 1c6d46293.  Fcntl ':mode' and
File::stat were added in c37b3d08c which was probably a leftover from an
earlier version of the patch, as the function using these was added to
PostgresNode in that commit.

> src/test/ldap/t/001_auth.pl| 1 -

The Config reference here was added in ee56c3b21 which in turn use $^O instead
of interrogating Config, so it can be removed as well.

I'll take it for a tour on the CI and will then apply.

--
Daniel Gustafsson   https://vmware.com/

Re: Document atthasmissing default optimization avoids verification table scan

[David G. Johnston]

On Sat, Mar 26, 2022 at 3:25 PM James Coleman  wrote:

> On Fri, Mar 25, 2022 at 4:40 PM Robert Haas  wrote:
> >
> > On Tue, Jan 25, 2022 at 8:49 AM James Coleman  wrote:
> > > Here's a version that looks like that. I'm not convinced it's an
> > > improvement over the previous version: again, I expect more advanced
> > > users to already understand this concept, and I think moving it to the
> > > ALTER TABLE page could very well have the effect of burying i(amidst
> > > the ton of detail on the ALTER TABLE page) concept that would be
> > > useful to learn early on in a tutorial like the DDL page. But if
> > > people really think this is an improvement, then I can acquiesce.
> >
> > I vote for rejecting both of these patches.
> >
> > 0001 adds the following sentence to the documentation: "A NOT
> > NULL constraint may be added to the new column in the same
> > statement without requiring scanning the table to verify the
> > constraint." My first reaction when I read this sentence was that it
> > was warning the user about the absence of a hazard that no one would
> > expect in the first place. We could also document that adding a NOT
> > NULL constraint will not cause your gas tank to catch fire, but nobody
> > was worried about that until we brought it up.
>
> As noted at minimum we (Braintree Payments) feared this hazard. That's
> reasonable because adding a NOT NULL constraint normally requires a
> table scan while holding an exclusive lock. It's fairly obvious why
> someone like us (any anyone who can't have downtime) would be paranoid
> about any possibility of long-running operations under exclusive locks
>
>
Reading the docs again I see:

ALTER TABLE ... ALTER COLUMN ... SET/DROP NOT NULL
"SET NOT NULL may only be applied to a column provided none of the records
in the table contain a NULL value for the column. Ordinarily this is
checked during the ALTER TABLE by scanning the entire table; however, if a
valid CHECK constraint is found which proves no NULL can exist, then the
table scan is skipped."

And the claim is that the reader would read this behavior of the ALTER
COLUMN ... SET NOT NULL command and assume that it might also apply to:

ALTER TABLE ... ADD COLUMN ... DEFAULT NOT NULL

I accept that such an assumption is plausible and worth disabusing
(regardless of my opinion, that is, to my understanding, why this patch is
being introduced).

To that end we should do so in the ALTER COLUMN ... SET NOT NULL section,
not the ADD COLUMN ... DEFAULT NOT NULL (or, specifically, its
corresponding paragraph in the notes section).

I would suggest rewriting 0001 to target ALTER COLUMN instead of in the
generic notes section (in the paragraph beginning "Adding a column with a
volatile DEFAULT") for the desired clarification.

0002, the moving of existing content from DDL to ALTER TABLE, does not have
agreement and the author of this patch isn't behind it.  I'm not inclined
to introduce a patch to push forth the discussion to conclusion myself.  So
for now it should just die.

David J.

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Thomas Munro]

On Sat, Mar 26, 2022 at 4:14 PM Justin Pryzby  wrote:
> I see it here (and in cfbot), although I'm not sure how you created a new
> patch for the active CF, and not for the next CF.

Anyone who has ever been a CF manager has this power, it seems.  I did
it myself once, by accident, and got told off by the active CF
manager.

Re: Document atthasmissing default optimization avoids verification table scan

[James Coleman]

On Fri, Mar 25, 2022 at 5:00 PM Tom Lane  wrote:
>
> Robert Haas  writes:
> > I vote for rejecting both of these patches.
>
> I see what James is on about here, but I agree that these specific changes
> don't help much.  What would actually be desirable IMO is a separate
> section somewhere explaining the performance characteristics of ALTER
> TABLE.  (We've also kicked around the idea of EXPLAIN for ALTER TABLE,
> but that's a lot more work.)  This could coalesce the parenthetical
> remarks that exist in ddl.sgml as well as alter_table.sgml into
> something a bit more unified and perhaps easier to follow.  In particular,
> it should start by defining what we mean by "table rewrite" and "table
> scan".  I don't recall at the moment whether we define those in multiple
> places or not at all, but as things stand any such discussion would be
> pretty fragmented.
>
> regards, tom lane

I think a unified area discussing pitfalls/performance of ALTER TABLE
seems like a great idea.

That being said: given that "as things stand" that "discussion
[already is] pretty fragmented" is there a place for a simpler
improvement (adding a short explanation of this particular hazard) in
the meantime? I don't mean this specific v4 patch -- just in general
(since the patch can be revised of course).

Thanks,
James Coleman

Re: Document atthasmissing default optimization avoids verification table scan

[James Coleman]

On Fri, Mar 25, 2022 at 4:40 PM Robert Haas  wrote:
>
> On Tue, Jan 25, 2022 at 8:49 AM James Coleman  wrote:
> > Here's a version that looks like that. I'm not convinced it's an
> > improvement over the previous version: again, I expect more advanced
> > users to already understand this concept, and I think moving it to the
> > ALTER TABLE page could very well have the effect of burying i(amidst
> > the ton of detail on the ALTER TABLE page) concept that would be
> > useful to learn early on in a tutorial like the DDL page. But if
> > people really think this is an improvement, then I can acquiesce.
>
> I vote for rejecting both of these patches.
>
> 0001 adds the following sentence to the documentation: "A NOT
> NULL constraint may be added to the new column in the same
> statement without requiring scanning the table to verify the
> constraint." My first reaction when I read this sentence was that it
> was warning the user about the absence of a hazard that no one would
> expect in the first place. We could also document that adding a NOT
> NULL constraint will not cause your gas tank to catch fire, but nobody
> was worried about that until we brought it up.

As noted at minimum we (Braintree Payments) feared this hazard. That's
reasonable because adding a NOT NULL constraint normally requires a
table scan while holding an exclusive lock. It's fairly obvious why
someone like us (any anyone who can't have downtime) would be paranoid
about any possibility of long-running operations under exclusive locks

I realize it's rhetorical flourish, but it hardly seems reasonable to
compare an actual hazard a database could plausibly have (an index it
is an optimization in the code that prevents it from happening -- a
naive implementation would in fact scan the full table on all NOT NULL
constraint additions) with something not at all related to database
(gas tank fires).

I simply do not accept the claim that this is not a reasonable concern
to have nor that this isn't worth documenting. It was worth someone
taking the time to consider as an optimization in the code. And the
consequence of that not having been done could be an outage for an
unsuspecting user. Of all the things we would want to document DDL
that could require executing long operations while holding exclusive
locks seems pretty high on the list.

> I also think that the
> sentence makes the rest of the paragraph harder to understand, because
> the rest of the paragraph is talking about adding a new column with a
> default, and now suddenly we're talking about NOT NULL constraints.

I am, however, happy to hear critiques of the style of the patch or
the best way to document this kind of behavior.

I'm curious though what you'd envision being a better place for this
information. Yes, we're talking about new columns -- that's the
operation under consideration -- but the NOT NULL constraint is part
of the new column definition. I'm not sure where else you would
document something that's a part of adding a new column.

> 0002 moves some advice about adding columns with defaults from one
> part of the documentation to another. Maybe that's a good idea, and
> maybe it isn't, but it also rewords the advice, and in my opinion, the
> new wording is less clear and specific than the existing wording. It
> also changes a sentence that mentions volatile defaults to give a
> specific example of a volatile function -- clock_timestamp -- probably
> because where the documentation was before that function was
> mentioned. However, that sentence seems clear enough as it is and does
> not really need an example.

Adding that example (and, indeed, moving the advice) was per a
previous reviewer's request. So I'm not sure what to do in this
situation -- I'm trying to improve the proposal per reviewer feedback
but there are conflicting reviewers. I suppose we'd need a
tie-breaking reviewer.

Thanks,
James Coleman

Re: Why is lorikeet so unstable in v14 branch only?

[Tom Lane]

Andrew Dunstan  writes:
> Yes it seems like a bug, but hard to diagnose. It seemed like a bug back
> in May:  see
> 

Ah, right, but that link is busted.  Here's the correct link:

https://www.postgresql.org/message-id/flat/e6f1fb3e-1e08-0188-9c71-2b5b894571de%40dunslane.net

> I vaguely theorize about a buffer overrun somewhere that scribbles on
> the stack.

I said in the earlier thread

> A platform-specific problem in get_ps_display() seems plausible
> enough.  The apparent connection to a concurrent VACUUM FULL seems
> pretty hard to explain that way ... but maybe that's a mirage.

but your one stack trace showed a crash while trying to lock pg_class for
ScanPgRelation, which'd potentially have blocked because of the VACUUM ---
and that'd result in a process title change, if not disabled.  So now
I feel like "something rotten in ps_status.c" is a theory that can fit
the available facts.

> If I understand correctly that you're only seeing this in v13 and
> HEAD, then it seems like bf68b79e5 (Refactor ps_status.c API)
> deserves a hard look.

I still stand by this opinion.  Can you verify which of the ps_status.c
code paths gets used on this build?

regards, tom lane

Re: Column Filtering in Logical Replication

[Tomas Vondra]

On 3/26/22 22:55, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 3/26/22 22:37, Tom Lane wrote:
>>> This smells like an uninitialized-variable problem, but I've had
>>> no luck finding any problem under valgrind.  Not sure how to progress
>>> from here.
> 
>> I think I see the problem - there's a CREATE SUBSCRIPTION but the test
>> is not waiting for the tablesync to complete, so sometimes it finishes
>> in time and sometimes not. That'd explain the flaky behavior, and it's
>> just this one test that misses the sync AFAICS.
> 
> Ah, that would also fit the symptoms.
> 

I'll go over the test to check if some other test misses that, and
perhaps do a bit of testing, and then push a fix.


regards

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

Re: Column Filtering in Logical Replication

[Tom Lane]

Tomas Vondra  writes:
> On 3/26/22 22:37, Tom Lane wrote:
>> This smells like an uninitialized-variable problem, but I've had
>> no luck finding any problem under valgrind.  Not sure how to progress
>> from here.

> I think I see the problem - there's a CREATE SUBSCRIPTION but the test
> is not waiting for the tablesync to complete, so sometimes it finishes
> in time and sometimes not. That'd explain the flaky behavior, and it's
> just this one test that misses the sync AFAICS.

Ah, that would also fit the symptoms.

regards, tom lane

Re: Column Filtering in Logical Replication

[Tomas Vondra]

On 3/26/22 22:37, Tom Lane wrote:
> Tomas Vondra  writes:
>> I went over the patch again, polished the commit message a bit, and
>> pushed. May the buildfarm be merciful!
> 
> Initial results aren't that great.  komodoensis[1], petalura[2],
> and snapper[3] have all shown variants of
> 
> #   Failed test 'partitions with different replica identities not replicated 
> correctly'
> #   at t/031_column_list.pl line 734.
> #  got: '2|4|
> # 4|9|'
> # expected: '1||5
> # 2|4|
> # 3||8
> # 4|9|'
> # Looks like you failed 1 test of 34.
> [18:19:36] t/031_column_list.pl ... 
> Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/34 subtests 
> 
> snapper reported different actual output than the other two:
> #  got: '1||5
> # 3||8'
> 
> The failure seems intermittent, as both komodoensis and petalura
> have also passed cleanly since the commit (snapper's only run once).
> 
> This smells like an uninitialized-variable problem, but I've had
> no luck finding any problem under valgrind.  Not sure how to progress
> from here.
> 

I think I see the problem - there's a CREATE SUBSCRIPTION but the test
is not waiting for the tablesync to complete, so sometimes it finishes
in time and sometimes not. That'd explain the flaky behavior, and it's
just this one test that misses the sync AFAICS.

FWIW I did run this under valgrind a number of times, and also on
various ARM machines that tend to trip over memory issues.


regards

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

Re: pg_stat_get_replication_slot() marked not strict, crashes

[Andres Freund]

Hi,

On 2022-03-26 17:41:53 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I wonder if we ought to make PG_GETARG_DATUM(n) assert that 
> > !PG_ARGISNULL(n)?
> > That'd perhaps make it easier to catch some of these...
> 
> Don't see the point; such cases will crash just fine without any
> assert.  The problem is lack of test coverage ...

Not reliably. Byval types typically won't crash, just do something
bogus. As e.g. in the case of pg_stat_get_subscription_stats(NULL) I found to
also be wrong upthread.

Greetings,

Andres Freund

Re: Why is lorikeet so unstable in v14 branch only?

[Andrew Dunstan]

On 3/26/22 17:19, Tom Lane wrote:
> Andrew Dunstan  writes:
>> On 3/26/22 15:49, Andres Freund wrote:
>>> One interesting bit in the config is:
>>> [ lack of ]
>>> 'update_process_title = off'
>> I'd forgotten about that. Let me do that for REL_14_STABLE and see where
>> we get to.
> Hm.  But if that does mitigate it, it still seems like a bug no?
> Why would that be preferentially crashing partitioned-table creation?


Yes it seems like a bug, but hard to diagnose. It seemed like a bug back
in May:  see


I vaguely theorize about a buffer overrun somewhere that scribbles on
the stack.

The answer to Andres's question about where the stackdumps go is that
they go in the data directory, AFAIK. You can see the buildfarm logic
for collecting them at

starting at line 149. There are various appropriate invocations of
get_stack_trace() in run_build.pl.


cheers


andrew



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

Re: pg_stat_get_replication_slot() marked not strict, crashes

[Tom Lane]

Andres Freund  writes:
> I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
> That'd perhaps make it easier to catch some of these...

Don't see the point; such cases will crash just fine without any
assert.  The problem is lack of test coverage ...

> It'd be nice to have a test in sanity check to just call each non-strict
> function with NULL inputs automatically. But the potential side-effects
> probably makes that not a realistic option?

... and as you say, brute force testing seems difficult.  I'm
particularly worried about multi-argument functions, as in
principle we'd need to check each argument separately, and cons
up something plausible to pass to the other arguments.

regards, tom lane

Re: Column Filtering in Logical Replication

[Tom Lane]

Tomas Vondra  writes:
> I went over the patch again, polished the commit message a bit, and
> pushed. May the buildfarm be merciful!

Initial results aren't that great.  komodoensis[1], petalura[2],
and snapper[3] have all shown variants of

#   Failed test 'partitions with different replica identities not replicated 
correctly'
#   at t/031_column_list.pl line 734.
#  got: '2|4|
# 4|9|'
# expected: '1||5
# 2|4|
# 3||8
# 4|9|'
# Looks like you failed 1 test of 34.
[18:19:36] t/031_column_list.pl ... 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/34 subtests 

snapper reported different actual output than the other two:
#  got: '1||5
# 3||8'

The failure seems intermittent, as both komodoensis and petalura
have also passed cleanly since the commit (snapper's only run once).

This smells like an uninitialized-variable problem, but I've had
no luck finding any problem under valgrind.  Not sure how to progress
from here.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis=2022-03-26%2015%3A54%3A04
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura=2022-03-26%2004%3A20%3A04
[3] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snapper=2022-03-26%2018%3A46%3A28

Re: More weird compiler warnings

[Andres Freund]

Hi,

On 2022-03-26 17:04:16 -0400, Tom Lane wrote:
> Hmm, looks like the gcc folk aren't too sure either ;-).

Heh, yea ;)

> Okay, so we can change this code, or just do nothing and wait for
> a repaired gcc.  Since that's an unreleased version there's no
> concern about any possible bug in-the-wild.  I think it probably
> should come down to whether we think the predecrement form is
> indeed more readable.

Agreed.


> I'm about +0.1 towards changing, what do you think?

Similar.


Greetings,

Andres Freund

pg_stat_get_replication_slot() marked not strict, crashes

[Andres Freund]

Hi,

I'm working to increase the test coverage of pgstat related stuff higher (for
the shared memory stats patch, of course).

"Accidentally" noticed that
  SELECT * FROM pg_stat_get_replication_slot(NULL);
crashes.  This is present in HEAD and 14.

I guess we'll have to add a code-level check in 14 to deal with this?


pg_stat_get_subscription_stats() also is wrongly marked. But at least in the
trivial cases just returns bogus results (for 0/InvalidOid). That's only in
HEAD, so easy to deal with.

The other functions returned by
  SELECT oid::regprocedure FROM pg_proc WHERE proname LIKE 'pg%stat%' AND 
pronargs > 0 AND NOT proisstrict;
look ok.


I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
That'd perhaps make it easier to catch some of these...

It'd be nice to have a test in sanity check to just call each non-strict
function with NULL inputs automatically. But the potential side-effects
probably makes that not a realistic option?

Greetings,

Andres Freund

Re: Why is lorikeet so unstable in v14 branch only?

[Tom Lane]

Andrew Dunstan  writes:
> On 3/26/22 15:49, Andres Freund wrote:
>> One interesting bit in the config is:
>> [ lack of ]
>> 'update_process_title = off'

> I'd forgotten about that. Let me do that for REL_14_STABLE and see where
> we get to.

Hm.  But if that does mitigate it, it still seems like a bug no?
Why would that be preferentially crashing partitioned-table creation?

regards, tom lane

Re: Refactoring SSL tests

[Daniel Gustafsson]

> On 9 Feb 2022, at 14:28, Andrew Dunstan  wrote:
> On 2/9/22 08:11, Daniel Gustafsson wrote:

>> Good point, done in the attached.
> 
> LGTM

Now that the recent changes to TAP and SSL tests have settled, I took another
pass at this.  After rebasing and fixing and polishing and taking it for
multiple spins on the CI I've pushed this today to master.

--
Daniel Gustafsson   https://vmware.com/

Re: More weird compiler warnings

[Tom Lane]

Andres Freund  writes:
> On 2022-03-26 16:23:26 -0400, Tom Lane wrote:
>> serinus' experimental gcc whines about a few places in network.c:

> I reported this to the gcc folks, that's clearly a bug. I suspect that it
> might not just cause spurious warnings, but also code generation issues - but
> I don't know that part for sure.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104986

Hmm, looks like the gcc folk aren't too sure either ;-).  But yeah,
given the discussion so far it's plausible there could be actually
bad code emitted.

>> but I'm wondering if we could silence the warning by changing the loop 
>> condition to
>>  while (--nb >= 0)
>> which seems like it might be marginally more readable anyway.

> Yes, that looks like it silences it.  I modified the small reproducer I had in
> that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes.

Okay, so we can change this code, or just do nothing and wait for
a repaired gcc.  Since that's an unreleased version there's no
concern about any possible bug in-the-wild.  I think it probably
should come down to whether we think the predecrement form is
indeed more readable.  I'm about +0.1 towards changing, what
do you think?

regards, tom lane

Re: Why is lorikeet so unstable in v14 branch only?

[Andrew Dunstan]

On 3/26/22 15:49, Andres Freund wrote:
> On 2022-03-26 14:47:07 -0400, Tom Lane wrote:
>> I chanced to notice that buildfarm member lorikeet has been
>> failing an awful lot lately in the v14 branch, but hardly
>> at all in other branches.  Here's a log extract from its
>> latest run [1]:
> One interesting bit in the config is:
>
>'extra_config' => {
>...
>'HEAD' => [
>'update_process_title = 
> off'
>  ],
>'REL_13_STABLE' => [
> 
> 'update_process_title = off'
>   ]
>

I'd forgotten about that. Let me do that for REL_14_STABLE and see where
we get to.


>> *** starting debugger for pid 53762, tid 10536
>> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:4] LOG:  server process (PID 
>> 53762) exited with exit code 127
>> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:5] DETAIL:  Failed process was 
>> running: create table mlparted_tab_part1 partition of mlparted_tab for 
>> values in (1);
>> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:6] LOG:  terminating any other 
>> active server processes
> I wonder what where the output of "starting debugger for pid 53762" ends up? I
> assume it's triggered by
> 'CYGWIN' => 'server 
> error_start=c:\\ncygwin64\\bin\\dumper.exe -d %1 %2',
>
> https://cygwin.org/cygwin-ug-net/using-cygwinenv.html
> says "The filename of the executing program and it's Windows process id are 
> appended to the command as arguments. "
>
> but nothing about %1 and %2 :(. I those are just "executing program" and
> "Windows process id" respectively?



I don't remember where I got this invocation from. But see for example



cheers


andrew


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

Re: More weird compiler warnings

[Andres Freund]

Hi,

On 2022-03-26 16:23:26 -0400, Tom Lane wrote:
> serinus' experimental gcc whines about a few places in network.c:
>
> ../../../../../pgsql/src/backend/utils/adt/network.c: In function 'inetnot':
> ../../../../../pgsql/src/backend/utils/adt/network.c:1893:34: warning: 
> writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>  1893 | pdst[nb] = ~pip[nb];
>   | ~^~
> ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into 
> destination object 'ipaddr' of size 16
>27 | unsigned char ipaddr[16];   /* up to 128 bits of address 
> */
>   |   ^~
> ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into 
> destination object 'ipaddr' of size 16
>
> The code in question looks like
>
> {
> intnb = ip_addrsize(ip);
> unsigned char *pip = ip_addr(ip);
> unsigned char *pdst = ip_addr(dst);
>
> while (nb-- > 0)
> pdst[nb] = ~pip[nb];
> }
>
> There's nothing actually wrong with this

I reported this to the gcc folks, that's clearly a bug. I suspect that it
might not just cause spurious warnings, but also code generation issues - but
I don't know that part for sure.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104986


> but I'm wondering if we could silence the warning by changing the loop 
> condition to
>
> while (--nb >= 0)
>
> which seems like it might be marginally more readable anyway.

Yes, that looks like it silences it.  I modified the small reproducer I had in
that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes.

Greetings,

Andres Freund

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Daniel Gustafsson]

> On 26 Mar 2022, at 21:24, Tom Lane  wrote:

> +1, but will it help for CI or buildfarm cases?

Isn't it both, but mostly for CI since the buildfarm already prints the path
when dumping the logfile.  Below is a random example snippet from the buildfarm
where it's fairly easy to see 001_basic.pl being the pg_test_fsync test:

/bin/prove -I ../../../src/test/perl/ -I . --timer t/*.pl
[20:31:18] t/001_basic.pl .. ok  224 ms ( 0.00 usr  0.01 sys +  0.18 cusr  
0.01 csys =  0.20 CPU)
[20:31:18]
All tests successful.
Files=1, Tests=12,  0 wallclock secs ( 0.05 usr  0.02 sys +  0.18 cusr  0.01 
csys =  0.26 CPU)
Result: PASS


== 
pgsql.build/src/bin/pg_test_fsync/tmp_check/log/regress_log_001_basic 
===
..

--
Daniel Gustafsson   https://vmware.com/

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Andres Freund]

Hi,

On 2022-03-26 16:24:32 -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Sat, Mar 26, 2022 at 3:35 PM Andres Freund  wrote:
> >> === tap tests in src/bin/pg_resetwal ===
> >> t/001_basic.pl .. ok
> >> t/002_corrupted.pl .. ok
> >> All tests successful.
>
> > Yeah, this certainly seems like an improvement to me.

Do we want to do the same of regress and isolation tests? They're mostly a bit
easier to place, but it's still a memory retention game.  Using the above
format for all looks a tad weird, due to pg_regress' output having kinda
similar markers.

...
==
 All 22 tests passed.
==

=== regress tests in contrib/ltree_plpython" ===
== creating temporary instance==
== initializing database system   ==
== starting postmaster==
running on port 51696 with PID 3905518
== creating database "contrib_regression" ==
CREATE DATABASE
ALTER DATABASE
== installing ltree   ==
CREATE EXTENSION
== running regression test queries==
test ltree_plpython   ... ok   51 ms
== shutting down postmaster   ==
== removing temporary instance==
...


Could just use a different character. +++ doesn't look bad:
+++ tap tests in contrib/test_decoding +++
t/001_repl_stats.pl .. ok
All tests successful.
Files=1, Tests=2,  3 wallclock secs ( 0.02 usr  0.00 sys +  1.74 cusr  0.28 
csys =  2.04 CPU)
Result: PASS


Would we want to do this in all branches? I'd vote for yes, but ...

Prototype patch attached. I looked through the uses of
  pg_(isolation_)?regress_(install)?check'
and didn't find any that'd have a problem with turning the invocation into a
multi-command one.


> +1, but will it help for CI

Yes, it should make it considerably better (for everything but windows, but
that outputs separators already).


> or buildfarm cases?

Probably not much, because that largely runs tests serially with "stage" names
corresponding to the test. And when it runs multiple tests in a row it adds
something similar to the above, e.g.:
=== Module pg_stat_statements check =
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=peripatus=2022-03-26%2000%3A20%3A30=misc-check

But I think it'll still be a tad better when it runs a single test:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=snapper=2022-03-26%2018%3A46%3A28=subscription-check


Might make it more realistic to make -s, at least to run tests? The reams of
output like:
gmake -C ../../../../src/test/regress pg_regress
gmake[1]: Entering directory 
'/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/test/regress'
gmake -C ../../../src/port all
gmake[2]: Entering directory 
'/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port'
gmake[2]: Nothing to be done for 'all'.
gmake[2]: Leaving directory 
'/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/port'
gmake -C ../../../src/common all
gmake[2]: Entering directory 
'/home/pgbuildfarm/buildroot/HEAD/pgsql.build/src/common'
gmake[2]: Nothing to be done for 'all'.

are quite clutter-y.

Greetings,

Andres Freund
diff --git i/src/Makefile.global.in w/src/Makefile.global.in
index 0726b2020ff..4014e437fc6 100644
--- i/src/Makefile.global.in
+++ w/src/Makefile.global.in
@@ -448,6 +448,7 @@ ifeq ($(enable_tap_tests),yes)
 
 ifndef PGXS
 define prove_installcheck
+echo "+++ tap tests in $(subdir) +++" && \
 rm -rf '$(CURDIR)'/tmp_check && \
 $(MKDIR_P) '$(CURDIR)'/tmp_check && \
 cd $(srcdir) && \
@@ -458,6 +459,7 @@ cd $(srcdir) && \
 endef
 else # PGXS case
 define prove_installcheck
+echo "+++ tap tests in $(subdir) +++" && \
 rm -rf '$(CURDIR)'/tmp_check && \
 $(MKDIR_P) '$(CURDIR)'/tmp_check && \
 cd $(srcdir) && \
@@ -469,6 +471,7 @@ endef
 endif # PGXS
 
 define prove_check
+echo "+++ tap tests in $(subdir) +++" && \
 rm -rf '$(CURDIR)'/tmp_check && \
 $(MKDIR_P) '$(CURDIR)'/tmp_check && \
 cd $(srcdir) && \
@@ -663,6 +666,7 @@ pg_regress_locale_flags = $(if $(ENCODING),--encoding=$(ENCODING)) $(NOLOCALE)
 pg_regress_clean_files = results/ regression.diffs regression.out tmp_check/ tmp_check_iso/ log/ output_iso/
 
 pg_regress_check = \
+echo "+++ regress tests in $(subdir) +++" && \
 $(with_temp_install) \
 $(top_builddir)/src/test/regress/pg_regress \
 --temp-instance=./tmp_check \
@@ -671,12 +675,14 @@ pg_regress_check = \
 $(TEMP_CONF) \
 $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS)
 pg_regress_installcheck = \
+echo "+++ regress tests in $(subdir) +++" && \
 $(top_builddir)/src/test/regress/pg_regress \
 --inputdir=$(srcdir) \
 --bindir='$(bindir)' \
 $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS)
 
 pg_isolation_regress_check = \
+echo "+++ isolation tests in $(subdir) +++" && \
 

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Tom Lane]

Robert Haas  writes:
> On Sat, Mar 26, 2022 at 3:35 PM Andres Freund  wrote:
>> === tap tests in src/bin/pg_resetwal ===
>> t/001_basic.pl .. ok
>> t/002_corrupted.pl .. ok
>> All tests successful.

> Yeah, this certainly seems like an improvement to me.

+1, but will it help for CI or buildfarm cases?

regards, tom lane

More weird compiler warnings

[Tom Lane]

serinus' experimental gcc whines about a few places in network.c:

../../../../../pgsql/src/backend/utils/adt/network.c: In function 'inetnot':
../../../../../pgsql/src/backend/utils/adt/network.c:1893:34: warning: writing 
1 byte into a region of size 0 [-Wstringop-overflow=]
 1893 | pdst[nb] = ~pip[nb];
  | ~^~
../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into 
destination object 'ipaddr' of size 16
   27 | unsigned char ipaddr[16];   /* up to 128 bits of address */
  |   ^~
../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into 
destination object 'ipaddr' of size 16

The code in question looks like

{
intnb = ip_addrsize(ip);
unsigned char *pip = ip_addr(ip);
unsigned char *pdst = ip_addr(dst);

while (nb-- > 0)
pdst[nb] = ~pip[nb];
}

There's nothing actually wrong with this, but I'm wondering if
we could silence the warning by changing the loop condition to

while (--nb >= 0)

which seems like it might be marginally more readable anyway.

regards, tom lane

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Daniel Gustafsson]

> On 26 Mar 2022, at 21:03, Robert Haas  wrote:
> 
> On Sat, Mar 26, 2022 at 3:35 PM Andres Freund  wrote:
>> === tap tests in src/bin/pg_resetwal ===
>> t/001_basic.pl .. ok
>> t/002_corrupted.pl .. ok
>> All tests successful.
>> Files=2, Tests=18,  3 wallclock secs ( 0.01 usr  0.01 sys +  2.39 cusr  0.31 
>> csys =  2.72 CPU)
>> Result: PASS
>> === tap tests in src/bin/pg_checksums ===
>> t/001_basic.pl  ok
>> t/002_actions.pl .. ok
>> All tests successful.
>> Files=2, Tests=74,  4 wallclock secs ( 0.02 usr  0.01 sys +  1.57 cusr  0.42 
>> csys =  2.02 CPU)
>> Result: PASS
> 
> Yeah, this certainly seems like an improvement to me.

+1, that's clearly better.

--
Daniel Gustafsson   https://vmware.com/

Re: Doc patch: replace 'salesmen' with 'salespeople'

[Daniel Gustafsson]

> On 25 Mar 2022, at 13:59, Dagfinn Ilmari Mannsåker  wrote:
> 
> Daniel Gustafsson  writes:
> 
>>> On 24 Mar 2022, at 19:34, Dagfinn Ilmari Mannsåker  
>>> wrote:
>> 
>>> I just spotted an unnecessarily gendered example involving a 'salesmen'
>>> table in the UPDATE docs. Here's a patch that changes that to
>>> 'salespeople'.
>> 
>> No objections to changing that, it's AFAICT the sole such usage in the docs.
> 
> There's a mention of the travelling salesman problem in the GEQO docs
> (and one in the code comments), but that's the established name for that
> problem (although I do note the Wikipedia page says it's "also called
> the travelling salesperson problem").

I would be slightly worried about "git grep'ability" when changing such an
established name (even though the risk might be miniscule here).  Unless it's
deemed controversial I would err on the side of caution and leave this alone.

>>>   Update contact names in an accounts table to match the currently assigned
>>> -   salesmen:
>>> +   salespeople:
>>> 
>>> UPDATE accounts SET (contact_first_name, contact_last_name) =
>>> -(SELECT first_name, last_name FROM salesmen
>>> - WHERE salesmen.id = accounts.sales_id);
>>> +(SELECT first_name, last_name FROM salespeople
>>> + WHERE salespeople.id = accounts.sales_id);
>> 
>> This example is a bit confusing to me, it's joining on accounts.sales_id to 
>> get
>> the assigned salesperson, but in the example just above we are finding the
>> salesperson by joining on accounts.sales_person.  Shouldn't this be using the
>> employees table to keep it consistent?  (which also avoids the gendered issue
>> raised here) The same goes for the second example. Or am I missing something?
> 
> Yeah, you're right. The second section (added by Tom in commit
> 8f889b1083f) is inconsistent with the first half in both table and
> column names. Here's a patch that makes it all consistent, eliminating
> the salesmen references completely, rather than renaming them.

I think this is an improvement, both in language and content.  The example does
show off a strange choice of schema but it's after all an example of syntax and
not data modelling. Barring objections I plan to go ahead with this.

--
Daniel Gustafsson   https://vmware.com/

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Robert Haas]

On Sat, Mar 26, 2022 at 3:35 PM Andres Freund  wrote:
> === tap tests in src/bin/pg_resetwal ===
> t/001_basic.pl .. ok
> t/002_corrupted.pl .. ok
> All tests successful.
> Files=2, Tests=18,  3 wallclock secs ( 0.01 usr  0.01 sys +  2.39 cusr  0.31 
> csys =  2.72 CPU)
> Result: PASS
> === tap tests in src/bin/pg_checksums ===
> t/001_basic.pl  ok
> t/002_actions.pl .. ok
> All tests successful.
> Files=2, Tests=74,  4 wallclock secs ( 0.02 usr  0.01 sys +  1.57 cusr  0.42 
> csys =  2.02 CPU)
> Result: PASS

Yeah, this certainly seems like an improvement to me.

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

Re: Why is lorikeet so unstable in v14 branch only?

[Andres Freund]

On 2022-03-26 14:47:07 -0400, Tom Lane wrote:
> I chanced to notice that buildfarm member lorikeet has been
> failing an awful lot lately in the v14 branch, but hardly
> at all in other branches.  Here's a log extract from its
> latest run [1]:

One interesting bit in the config is:

   'extra_config' => {
   ...
   'HEAD' => [
   'update_process_title = off'
 ],
   'REL_13_STABLE' => [

'update_process_title = off'
  ]


> *** starting debugger for pid 53762, tid 10536
> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:4] LOG:  server process (PID 
> 53762) exited with exit code 127
> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:5] DETAIL:  Failed process was 
> running: create table mlparted_tab_part1 partition of mlparted_tab for values 
> in (1);
> 2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:6] LOG:  terminating any other 
> active server processes

I wonder what where the output of "starting debugger for pid 53762" ends up? I
assume it's triggered by
'CYGWIN' => 'server 
error_start=c:\\ncygwin64\\bin\\dumper.exe -d %1 %2',

https://cygwin.org/cygwin-ug-net/using-cygwinenv.html
says "The filename of the executing program and it's Windows process id are 
appended to the command as arguments. "

but nothing about %1 and %2 :(. I those are just "executing program" and
"Windows process id" respectively?

Greetings,

Andres Freund

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Andres Freund]

Hi,

On 2022-03-26 14:40:06 -0400, Robert Haas wrote:
> Well, I think that's unwarranted. Many years ago, people discovered
> that it was annoying if you had to distinguish files solely based on
> name, and so they invented directories and pathnames. That was a good
> call.

Yea. I have no problem naming tests the same way, particularly if they do
similar things. But we should show the path.


> Displaying that information in the buildfarm output would be a good call,
> too.

I would find it very useful locally when running the tests too. A very simple
approach would be to invoke prove with absolute paths to the tests. But that's
not particularly pretty.  But unless we change the directory that prove is run
in away from the directory that contains t/ (there's a thread about that, but
more to do), I don't think we can do better on an individual test basis?

We could just make prove_[install]check echo the $(subdir) it's about to run
tests for? Certainly looks better to me:

make -j48 -Otarget -s -C src/bin/ check NO_TEMP_INSTALL=1
...
=== tap tests in src/bin/pg_resetwal ===
t/001_basic.pl .. ok
t/002_corrupted.pl .. ok
All tests successful.
Files=2, Tests=18,  3 wallclock secs ( 0.01 usr  0.01 sys +  2.39 cusr  0.31 
csys =  2.72 CPU)
Result: PASS
=== tap tests in src/bin/pg_checksums ===
t/001_basic.pl  ok
t/002_actions.pl .. ok
All tests successful.
Files=2, Tests=74,  4 wallclock secs ( 0.02 usr  0.01 sys +  1.57 cusr  0.42 
csys =  2.02 CPU)
Result: PASS
=== tap tests in src/bin/psql ===
t/001_basic.pl ... ok
t/010_tab_completion.pl .. ok
t/020_cancel.pl .. ok
All tests successful.
Files=3, Tests=125,  6 wallclock secs ( 0.03 usr  0.00 sys +  3.65 cusr  0.56 
csys =  4.24 CPU)
Result: PASS
...

Greetings,

Andres Freund

Re: Pointer subtraction with a null pointer

[Tom Lane]

Andres Freund  writes:
> Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in
> relptr_access too, think that'd be only one left over...

Hm.  Probably not worth the trouble, because it's hard to envision
a situation where rp is not a plain lvalue.

regards, tom lane

Why is lorikeet so unstable in v14 branch only?

[Tom Lane]

I chanced to notice that buildfarm member lorikeet has been
failing an awful lot lately in the v14 branch, but hardly
at all in other branches.  Here's a log extract from its
latest run [1]:

2022-03-26 06:31:47.245 EDT [623eeb93.d202:131] pg_regress/inherit LOG:  
statement: create table mlparted_tab (a int, b char, c text) partition by list 
(a);
2022-03-26 06:31:47.247 EDT [623eeb93.d202:132] pg_regress/inherit LOG:  
statement: create table mlparted_tab_part1 partition of mlparted_tab for values 
in (1);
2022-03-26 06:31:47.254 EDT [623eeb93.d203:60] pg_regress/vacuum LOG:  
statement: VACUUM FULL pg_class;
2022-03-26 06:31:47.258 EDT [623eeb92.d201:90] pg_regress/typed_table LOG:  
statement: SELECT a.attname,
  pg_catalog.format_type(a.atttypid, a.atttypmod),
  (SELECT pg_catalog.pg_get_expr(d.adbin, d.adrelid, true)
   FROM pg_catalog.pg_attrdef d
   WHERE d.adrelid = a.attrelid AND d.adnum = a.attnum AND a.atthasdef),
  a.attnotnull,
  (SELECT c.collname FROM pg_catalog.pg_collation c, pg_catalog.pg_type 
t
   WHERE c.oid = a.attcollation AND t.oid = a.atttypid AND 
a.attcollation <> t.typcollation) AS attcollation,
  a.attidentity,
  a.attgenerated
FROM pg_catalog.pg_attribute a
WHERE a.attrelid = '21770' AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY a.attnum;
*** starting debugger for pid 53762, tid 10536
2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:4] LOG:  server process (PID 53762) 
exited with exit code 127
2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:5] DETAIL:  Failed process was 
running: create table mlparted_tab_part1 partition of mlparted_tab for values 
in (1);
2022-03-26 06:32:02.158 EDT [623eeb6c.d0c2:6] LOG:  terminating any other 
active server processes

The failures are not all exactly like this one, but they're mostly in
CREATE TABLE operations nearby to this one.  I speculate what is happening
is that the "VACUUM FULL pg_class" is triggering some misbehavior in
concurrent partitioned-table creation.  The lack of failures in other
branches could be due to changes in the relative timing of the "vacuum"
and "inherit" test scripts.

Any chance we could get a stack trace from one of these crashes?

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet=2022-03-26%2010%3A17%3A22

Re: Pointer subtraction with a null pointer

[Andres Freund]

Hi,

On 2022-03-26 14:13:56 -0400, Tom Lane wrote:
> The attached also silences the warning, and getting rid of the double-eval
> hazard seems like a net win.

Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in
relptr_access too, think that'd be only one left over...


> > Might also be worth adding an assertion that base < val.
> 
> Did that too.  On the whole I like this better.

Better than the relptr_store_null() approach I assume? Agreed, if so.

Greetings,

Andres Freund

Re: pgsql: Add 'basebackup_to_shell' contrib module.

[Robert Haas]

On Fri, Mar 25, 2022 at 4:09 PM Andrew Dunstan  wrote:
> Duplication of TAP test names has long been something that's annoyed me.

Well, I think that's unwarranted. Many years ago, people discovered
that it was annoying if you had to distinguish files solely based on
name, and so they invented directories and pathnames. That was a good
call. Displaying that information in the buildfarm output would be a
good call, too.

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

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Andres Freund]

Hi,

On 2022-03-26 13:57:39 -0400, Tom Lane wrote:
> Seems like making it a GUC does nothing to fix the complaint you had about
> passing data to workers whether it's needed or not.

I don't think that was my complaint. Maybe Robert's?


> Sure, we don't then need to write any new code for it, but it's still new
> cycles.

I think it'd quite possibly less cycles than separately syncing it.

Because I wanted to know what the overhead be in relation to other things, I
made serialize_variable() log whenever it decides to serialize, and it's a bit
depressing :/.

serialized DateStyle = ISO, MDY
serialized default_text_search_config = pg_catalog.english
serialized force_parallel_mode = on
serialized lc_messages = en_US.UTF-8
serialized lc_monetary = en_US.UTF-8
serialized lc_numeric = en_US.UTF-8
serialized lc_time = en_US.UTF-8
serialized log_checkpoints = true
serialized log_line_prefix = %m [%p][%b][%v:%x][%a]
serialized log_timezone = America/Los_Angeles
serialized max_stack_depth = 2048
serialized max_wal_size = 153600
serialized min_wal_size = 48
serialized restart_after_crash = true
serialized session_authorization = andres
serialized ssl_cert_file = /home/andres/tmp/pgdev/ssl-cert-snakeoil.pem
serialized ssl_key_file = /home/andres/tmp/pgdev/ssl-cert-snakeoil.key
serialized TimeZone = America/Los_Angeles
serialized timezone_abbreviations = Default
serialized track_io_timing = true
serialized transaction_deferrable = false
serialized transaction_isolation = read committed
serialized transaction_read_only = false
total serialized guc state is 1324

Of course, compared to the total size of 94784 bytes that's not too
much... FWIW, 65536 of that is for the tuple queues...


> I also note that exposing it as a GUC means we have zero control over
> who/what can read it.  Maybe that's not a problem, but it needs to be
> thought about before we go down that path.

Yes, I think that's a fair concern.

Greetings,

Andres Freund

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Sat, Mar 26, 2022 at 11:21:56AM -0500, Justin Pryzby wrote:
> You're passing both the compression method *and* level.  I think there should
> be a structure which includes both.  In the future, that can also handle
> additional options.

I'm not sure if there's anything worth saving, but I did that last year with
0003-Support-multiple-compression-algs-levels-opts.patch
I sent a rebased copy off-list.
https://www.postgresql.org/message-id/flat/20210104025321.ga9...@telsasoft.com#ca1b9f9d3552c87fa874731cad9d8391

|   fatal("not built with LZ4 support");
|   fatal("not built with lz4 support");

Please use consistent capitalization of "lz4" - then the compiler can optimize
away duplicate strings.

> 0004: The include should use  and not "lz4.h"

Also, use USE_LZ4 rather than HAVE_LIBLZ4, per 75eae0908.

Re: Document atthasmissing default optimization avoids verification table scan

[Robert Haas]

On Fri, Mar 25, 2022 at 5:00 PM Tom Lane  wrote:
> I see what James is on about here, but I agree that these specific changes
> don't help much.  What would actually be desirable IMO is a separate
> section somewhere explaining the performance characteristics of ALTER
> TABLE.

Sure. If someone wants to do that and bring it to a level of quality
that we could consider committing, I'm fine with that. But I don't
think that has much to do with the patches before us.

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

Re: Column Filtering in Logical Replication

[Tomas Vondra]

On 3/26/22 10:58, Tomas Vondra wrote:
> On 3/26/22 05:09, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
>> Hello, 
>>
>> The 'prattrs' column has been added to the pg_publication_rel catalog, 
>> but the current commit to catalog.sgml seems to have added it to 
>> pg_publication_namespace. 
>> The attached patch fixes this.
>>
> 
> Thanks, I'll get this pushed.
> 

Pushed. Thanks for noticing this!


regards

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

Re: Pointer subtraction with a null pointer

[Tom Lane]

Andres Freund  writes:
> Wonder if we should try to get rid of the problem by also fixing the double
> evaluation of val? I think something like

Good idea.  The attached also silences the warning, and getting rid
of the double-eval hazard seems like a net win.

> Might also be worth adding an assertion that base < val.

Did that too.  On the whole I like this better.

regards, tom lane

diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index fdc2124d2c..cc80a7200d 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -56,11 +56,24 @@
 #define relptr_is_null(rp) \
 	((rp).relptr_off == 0)
 
+/* We use this inline to avoid double eval of "val" in relptr_store */
+static inline Size
+relptr_store_eval(char *base, char *val)
+{
+	if (val == NULL)
+		return 0;
+	else
+	{
+		Assert(val > base);
+		return val - base;
+	}
+}
+
 #ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
 #define relptr_store(base, rp, val) \
 	(AssertVariableIsOfTypeMacro(base, char *), \
 	 AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
-	 (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+	 (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
 #else
 /*
  * If we don't have __builtin_types_compatible_p, assume we might not have
@@ -68,7 +81,7 @@
  */
 #define relptr_store(base, rp, val) \
 	(AssertVariableIsOfTypeMacro(base, char *), \
-	 (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+	 (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
 #endif
 
 #define relptr_copy(rp1, rp2) \

Re: Pointer subtraction with a null pointer

[Andres Freund]

Hi,

On 2022-03-26 10:49:53 -0700, Andres Freund wrote:
> > It's hard to see how that isn't a flat-out compiler bug.
> 
> It only happens if the NULL is directly passed as an argument to the macro,
> not if there's an intermediary variable. Argh.
> 
> 
> #include 
> 
> #define relptr_store(base, rp, val) \
>((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
> 
> typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;
> 
> void
> problem_not_present(relptr *rp, char *base)
> {
> struct foo *val = NULL;
> 
> relptr_store(base, *rp, val);
> }
> 
> void
> problem_present(relptr *rp, char *base)
> {
> relptr_store(base, *rp, NULL);
> }
> 
> 
> Looks like that warning is uttered whenever there's a subtraction from a
> pointer with NULL, even if the code isn't reachable. Which I guess makes
> *some* sense, outside of macros it's not something that'd ever be reasonable.

Reported as https://github.com/llvm/llvm-project/issues/54570

Greetings,

Andres Freund

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Tom Lane]

Andres Freund  writes:
> On 2022-03-26 15:18:59 +0900, Michael Paquier wrote:
>> On Thu, Mar 24, 2022 at 05:44:06PM +, Jacob Champion wrote:
>>> On Wed, 2022-03-23 at 16:54 -0700, Andres Freund wrote:
 Another option would be to make it a GUC. With a bit of care it could be
 automatically synced by the existing parallelism infrastructure...

>>> Like a write-once, PGC_INTERNAL setting?

> Perhaps PGC_INTERNAL, perhaps PGC_SU_BACKEND, set with PGC_S_OVERRIDE?

Seems like making it a GUC does nothing to fix the complaint you had about
passing data to workers whether it's needed or not.  Sure, we don't then
need to write any new code for it, but it's still new cycles.  And it's
new cycles all throughout guc.c, too, not just in parallel worker start.

I also note that exposing it as a GUC means we have zero control over
who/what can read it.  Maybe that's not a problem, but it needs to be
thought about before we go down that path.

regards, tom lane

Re: Remove an unused function GetWalRcvWriteRecPtr

[Tom Lane]

Bharath Rupireddy  writes:
> On Sat, Mar 26, 2022 at 10:57 PM Andres Freund  wrote:
>> -1. I think it's a perfectly reasonable function to have, it doesn't cause
>> architectural / maintenance issues to have it and there's several plausible
>> future uses for it (moving fsyncing of received WAL to different process,
>> optionally allowing logical decoding up to the written LSN, reporting 
>> function
>> for monitoring on the standby itself).

> Given the use-cases that it may have in future, I can use that
> function right now in pg_stat_get_wal_receiver instead of
> pg_atomic_read_u64(>writtenUpto);

I do not really see a reason to change anything at all here.
We have far better things to spend our (finite) time on.

regards, tom lane

Re: Pointer subtraction with a null pointer

[Tom Lane]

I wrote:
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null.  It's hard
> to see how that isn't a flat-out compiler bug.
> However, granting that it isn't going to get fixed right away,
> we could replace these call sites with "relptr_store_null()",
> and maybe get rid of the conditional in relptr_store().

I've confirmed that the attached silences the warning with clang
13.0.0 (on Fedora 35).  The store_null notation is not awful, perhaps;
it makes those lines shorter and more readable.

I'm a bit less enthused about removing the conditional in relptr_store,
as that forces re-introducing it at a couple of call sites.  Perhaps
we should leave relptr_store alone ... but then the reason for
relptr_store_null is hard to explain except as a workaround for a
broken compiler.

I changed the comment suggesting that you could use relptrs with the
"process address space" as a base, because that would presumably mean
base == NULL which is going to draw the same warning.

regards, tom lane

diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/freepage.c
index b26a295a4e..7af8ec2283 100644
--- a/src/backend/utils/mmgr/freepage.c
+++ b/src/backend/utils/mmgr/freepage.c
@@ -185,8 +185,8 @@ FreePageManagerInitialize(FreePageManager *fpm, char *base)
 	Size		f;
 
 	relptr_store(base, fpm->self, fpm);
-	relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
-	relptr_store(base, fpm->btree_recycle, (FreePageSpanLeader *) NULL);
+	relptr_store_null(base, fpm->btree_root);
+	relptr_store_null(base, fpm->btree_recycle);
 	fpm->btree_depth = 0;
 	fpm->btree_recycle_count = 0;
 	fpm->singleton_first_page = 0;
@@ -198,7 +198,7 @@ FreePageManagerInitialize(FreePageManager *fpm, char *base)
 #endif
 
 	for (f = 0; f < FPM_NUM_FREELISTS; f++)
-		relptr_store(base, fpm->freelist[f], (FreePageSpanLeader *) NULL);
+		relptr_store_null(base, fpm->freelist[f]);
 }
 
 /*
@@ -596,7 +596,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
 			if (root->hdr.magic == FREE_PAGE_LEAF_MAGIC)
 			{
 /* If root is a leaf, convert only entry to singleton range. */
-relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
+relptr_store_null(base, fpm->btree_root);
 fpm->singleton_first_page = root->u.leaf_key[0].first_page;
 fpm->singleton_npages = root->u.leaf_key[0].npages;
 			}
@@ -608,7 +608,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
 Assert(root->hdr.magic == FREE_PAGE_INTERNAL_MAGIC);
 relptr_copy(fpm->btree_root, root->u.internal_key[0].child);
 newroot = relptr_access(base, fpm->btree_root);
-relptr_store(base, newroot->hdr.parent, (FreePageBtree *) NULL);
+relptr_store_null(base, newroot->hdr.parent);
 			}
 			FreePageBtreeRecycle(fpm, fpm_pointer_to_page(base, root));
 		}
@@ -634,8 +634,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
 	fpm->singleton_npages = root->u.leaf_key[0].npages +
 		root->u.leaf_key[1].npages + 1;
 	fpm->btree_depth = 0;
-	relptr_store(base, fpm->btree_root,
- (FreePageBtree *) NULL);
+	relptr_store_null(base, fpm->btree_root);
 	FreePagePushSpanLeader(fpm, fpm->singleton_first_page,
 		   fpm->singleton_npages);
 	Assert(max_contiguous_pages == 0);
@@ -886,8 +885,12 @@ FreePageBtreeGetRecycled(FreePageManager *fpm)
 	Assert(victim != NULL);
 	newhead = relptr_access(base, victim->next);
 	if (newhead != NULL)
+	{
 		relptr_copy(newhead->prev, victim->prev);
-	relptr_store(base, fpm->btree_recycle, newhead);
+		relptr_store(base, fpm->btree_recycle, newhead);
+	}
+	else
+		relptr_store_null(base, fpm->btree_recycle);
 	Assert(fpm_pointer_is_page_aligned(base, victim));
 	fpm->btree_recycle_count--;
 	return (FreePageBtree *) victim;
@@ -940,8 +943,11 @@ FreePageBtreeRecycle(FreePageManager *fpm, Size pageno)
 	span = (FreePageSpanLeader *) fpm_page_to_pointer(base, pageno);
 	span->magic = FREE_PAGE_SPAN_LEADER_MAGIC;
 	span->npages = 1;
-	relptr_store(base, span->next, head);
-	relptr_store(base, span->prev, (FreePageSpanLeader *) NULL);
+	if (head != NULL)
+		relptr_store(base, span->next, head);
+	else
+		relptr_store_null(base, span->next);
+	relptr_store_null(base, span->prev);
 	if (head != NULL)
 		relptr_store(base, head->prev, span);
 	relptr_store(base, fpm->btree_recycle, span);
@@ -998,7 +1004,7 @@ FreePageBtreeRemovePage(FreePageManager *fpm, FreePageBtree *btp)
 		if (parent == NULL)
 		{
 			/* We are removing the root page. */
-			relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
+			relptr_store_null(base, fpm->btree_root);
 			fpm->btree_depth = 0;
 			Assert(fpm->singleton_first_page == 0);
 			Assert(fpm->singleton_npages == 0);
@@ -1537,7 +1543,7 @@ FreePageManagerPutInternal(FreePageManager *fpm, Size first_page, Size npages,
 			/* Create the btree and move the preexisting range into it. */
 			root->hdr.magic = FREE_PAGE_LEAF_MAGIC;
 			root->hdr.nused = 1;
-			

Re: Pointer subtraction with a null pointer

[Andres Freund]

Hi,

On 2022-03-26 13:23:34 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund  writes:
> >> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
> >>> This code is old, but mylodon wasn't doing that a week ago, so
> >>> Andres must've updated the compiler and/or changed its options.
>
> >> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.
>
> > OK, that answers that.
>
> ... Actually, after looking closer, I misread what our code is doing.
> These call sites are trying to set the relptr value to "null" (zero),
> and AFAICS it should be allowed:
>
> freepage.c:188:2: warning: performing pointer subtraction with a null pointer 
> has undefined behavior [-Wnull-pointer-subtraction]
> relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
> ^~~
> ../../../../src/include/utils/relptr.h:63:59: note: expanded from macro 
> 'relptr_store'
>  (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
>  ^
>
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null.

Huh, yea. I somehow read the conditional branch as guarding against a an
uninitialized base pointer or such.


> It's hard to see how that isn't a flat-out compiler bug.

It only happens if the NULL is directly passed as an argument to the macro,
not if there's an intermediary variable. Argh.


#include 

#define relptr_store(base, rp, val) \
 ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))

typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;

void
problem_not_present(relptr *rp, char *base)
{
struct foo *val = NULL;

relptr_store(base, *rp, val);
}

void
problem_present(relptr *rp, char *base)
{
relptr_store(base, *rp, NULL);
}


Looks like that warning is uttered whenever there's a subtraction from a
pointer with NULL, even if the code isn't reachable. Which I guess makes
*some* sense, outside of macros it's not something that'd ever be reasonable.


Wonder if we should try to get rid of the problem by also fixing the double
evaluation of val? I think something like

static inline void
relptr_store_internal(size_t *off, char *base, char *val)
{
if (val == NULL)
*off = 0;
else
*off = val - base;
}

#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
 AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
 relptr_store_internal(&(rp).relptr_off, base, (char *) val))
#else
...

should do the trick?

Might also be worth adding an assertion that base < val.


> However, granting that it isn't going to get fixed right away,
> we could replace these call sites with "relptr_store_null()",
> and maybe get rid of the conditional in relptr_store().

Also would be good with that.

Greetings,

Andres Freund

Re: pg_stat_statements and "IN" conditions

[Dmitry Dolgov]

> On Mon, Mar 14, 2022 at 04:51:50PM +0100, Dmitry Dolgov wrote:
> > On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote:
> > Dmitry Dolgov <9erthali...@gmail.com> writes:
> > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote:
> > >> I do find it odd that the proposed patch doesn't cause the *entire*
> > >> list to be skipped over.  That seems like extra complexity and confusion
> > >> to no benefit.
> >
> > > That's a bit surprising for me, I haven't even thought that folks could
> > > think this is an odd behaviour. As I've mentioned above, the original
> > > idea was to give some clues about what was inside the collapsed array,
> > > but if everyone finds it unnecessary I can of course change it.
> >
> > But if what we're doing is skipping over an all-Consts list, then the
> > individual Consts would be elided from the pg_stat_statements entry
> > anyway, no?  All that would remain is information about how many such
> > Consts there were, which is exactly the information you want to drop.
>
> Hm, yes, you're right. I guess I was thinking about this more like about
> shortening some text with ellipsis, but indeed no actual Consts will end
> up in the result anyway. Thanks for clarification, will modify the
> patch!

Here is another iteration. Now the patch doesn't leave any trailing
Consts in the normalized query, and contains more documentation. I hope
it's getting better.
>From 8226635c221a659097deb6ea64626a587296ea60 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 12 Mar 2022 14:42:02 +0100
Subject: [PATCH v7] Prevent jumbling of every element in ArrayExpr

pg_stat_statements produces multiple entries for queries like

SELECT something FROM table WHERE col IN (1, 2, 3, ...)

depending on number of parameters, because every element of ArrayExpr is
jumbled. In certain situations it's undesirable, especially if the list
becomes too large.

Make Const expressions contribute nothing to the jumble hash if they're
a part of an ArrayExpr, which length is larger than specified threshold.
Allow to configure the threshold via the new GUC const_merge_threshold
with the default value zero, which disables this feature.

Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane
Tested-by: Chengxi Sun
---
 .../expected/pg_stat_statements.out   | 412 ++
 .../pg_stat_statements/pg_stat_statements.c   |  33 +-
 .../sql/pg_stat_statements.sql| 107 +
 doc/src/sgml/config.sgml  |  26 ++
 doc/src/sgml/pgstatstatements.sgml|  28 +-
 src/backend/utils/misc/guc.c  |  13 +
 src/backend/utils/misc/queryjumble.c  | 105 -
 src/include/utils/queryjumble.h   |   5 +-
 8 files changed, 711 insertions(+), 18 deletions(-)

diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..de3970e462 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -1077,4 +1077,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%';
  2
 (1 row)
 
+--
+-- Consts merging
+--
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--
+ 
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data 
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data 
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data 
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data 
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data 
++--
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query  | calls 
++---
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6)  | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7)  | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8)  | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9)  | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
+ SELECT pg_stat_statements_reset()  | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(7 rows)
+
+-- Normal
+SET const_merge_threshold = 5;
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--
+ 
+(1 row)
+
+SELECT 

Re: Remove an unused function GetWalRcvWriteRecPtr

[Bharath Rupireddy]

On Sat, Mar 26, 2022 at 10:57 PM Andres Freund  wrote:
>
> Hi,
>
> On 2022-03-26 10:51:15 +0530, Bharath Rupireddy wrote:
> > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without
> > spinlock) is being used directly in pg_stat_get_wal_receiver
> > walreceiver.c. We either make use of the function instead of
> > pg_atomic_read_u64(>writtenUpto); or remove it. Since there's
> > only one function using walrcv->writtenUpto right now, I prefer to
> > remove the function to save some LOC (13).
>
> -1. I think it's a perfectly reasonable function to have, it doesn't cause
> architectural / maintenance issues to have it and there's several plausible
> future uses for it (moving fsyncing of received WAL to different process,
> optionally allowing logical decoding up to the written LSN, reporting function
> for monitoring on the standby itself).

Given the use-cases that it may have in future, I can use that
function right now in pg_stat_get_wal_receiver instead of
pg_atomic_read_u64(>writtenUpto);

I was also thinking of a function to expose it but backed off because
it can't be used reliably for data integrity checks or do we want to
specify about this in the functions docs as well leave it to the user?
Thoughts?

/*
 * Like flushedUpto, but advanced after writing and before flushing,
 * without the need to acquire the spin lock.  Data can be read by another
 * process up to this point, but shouldn't be used for data integrity
 * purposes.
 */
pg_atomic_uint64 writtenUpto;

Regards,
Bharath Rupireddy.

Re: Remove an unused function GetWalRcvWriteRecPtr

[Andres Freund]

Hi,

On 2022-03-26 10:51:15 +0530, Bharath Rupireddy wrote:
> The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without
> spinlock) is being used directly in pg_stat_get_wal_receiver
> walreceiver.c. We either make use of the function instead of
> pg_atomic_read_u64(>writtenUpto); or remove it. Since there's
> only one function using walrcv->writtenUpto right now, I prefer to
> remove the function to save some LOC (13).

-1. I think it's a perfectly reasonable function to have, it doesn't cause
architectural / maintenance issues to have it and there's several plausible
future uses for it (moving fsyncing of received WAL to different process,
optionally allowing logical decoding up to the written LSN, reporting function
for monitoring on the standby itself).

Greetings,

Andres Freund

Re: Pointer subtraction with a null pointer

[Tom Lane]

I wrote:
> Andres Freund  writes:
>> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
>>> This code is old, but mylodon wasn't doing that a week ago, so
>>> Andres must've updated the compiler and/or changed its options.

>> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

> OK, that answers that.

... Actually, after looking closer, I misread what our code is doing.
These call sites are trying to set the relptr value to "null" (zero),
and AFAICS it should be allowed:

freepage.c:188:2: warning: performing pointer subtraction with a null pointer 
has undefined behavior [-Wnull-pointer-subtraction]
relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
^~~
../../../../src/include/utils/relptr.h:63:59: note: expanded from macro 
'relptr_store'
 (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
 ^

clang is complaining about the subtraction despite it being inside
a conditional arm that cannot be reached when val is null.  It's hard
to see how that isn't a flat-out compiler bug.

However, granting that it isn't going to get fixed right away,
we could replace these call sites with "relptr_store_null()",
and maybe get rid of the conditional in relptr_store().

regards, tom lane

Re: make MaxBackends available in _PG_init

[Andres Freund]

Hi,

On 2022-03-26 15:22:03 +0800, Julien Rouhaud wrote:
> On Fri, Mar 25, 2022 at 03:23:17PM -0700, Andres Freund wrote:
> >
> > I don't really understand. The issue that started this thread was bugs in
> > extensions due to accessing MaxBackends before it is initialized - which the
> > patch prevents.
> 
> Well, the patch prevents accessing a 0-valued MaxBackends but doesn't do
> anything to solve the original complaint.  It's not like extensions won't need
> to access that information during _PG_init anymore.

It resolves the pretty common bug that an extension breaks once it's used via
s_p_l instead of loaded on-demand because MaxBackends isn't initialized in the
s_p_l case.


> And indeed, any third party code that previously needed to access what
> MaxBackends is supposed to store should already be using that formula, and
> the new GetMaxBackends() doesn't do anything about it.

It couldn't rely on MaxBackends before. It can't rely on GetMaxBackends()
now. You can see why I think that what you want is unrelated to the
introduction of GetMaxBackends().

If we introduce a separate hook that allows to influence things like
max_connections or whatnot we'd *even more* need a way to verify whether it's
legal to access MaxBackends in that moment.


> So all extensions will be as broken as before, except the few that were
> using MaxBackends without realizing it's 0. And if those exist (there's
> actually one) they're not that broken, probably because MaxBackend is only
> used to request additional shmem, with wanted value small enough so that
> it's compensated by the extra 100kB shmem postgres allocates.

I don't think it's rare at all.

Greetings,

Andres Freund

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Andres Freund]

Hi,

On 2022-03-26 15:18:59 +0900, Michael Paquier wrote:
> On Thu, Mar 24, 2022 at 05:44:06PM +, Jacob Champion wrote:
> > On Wed, 2022-03-23 at 16:54 -0700, Andres Freund wrote:
> >> Another option would be to make it a GUC. With a bit of care it could be
> >> automatically synced by the existing parallelism infrastructure...
> >
> > Like a write-once, PGC_INTERNAL setting?

Perhaps PGC_INTERNAL, perhaps PGC_SU_BACKEND, set with PGC_S_OVERRIDE?


> > I guess I don't have any
> > intuition on how that would compare to the separate-global-and-accessor
> > approach. Is the primary advantage that you don't have to maintain the
> > serialization logic, or is there more to it?
>
> Hmm.  That would be a first for a GUC, no?  It is not seem natural
> compared to the other information pieces passed down from the leader
> to the workers.

What would be the first for a GUC? We have plenty GUCs that are set on a
per-connection basis to reflect some fact? And there's several authenitcation
related bits of state known to guc.c , think role, session_authorization,
is_superuser.

Sharing per-connection state via GUCs for paralellism? I don't think that is
true either. E.g. application_name, client_encoding.


> +extern SharedPort MyProcShared;

I strongly dislike MyProcShared. It's way too easily confused with MyProc
which point to shared memory.

Greetings,

Andres Freund

Re: pg_dump new feature: exporting functions only. Bad or good idea ?

[David G. Johnston]

On Sat, Mar 26, 2022 at 1:53 AM Laetitia Avrot 
wrote:

> Hello all,
>
> Le sam. 26 mars 2022 à 01:13, Michael Paquier  a
> écrit :
>
>> On Fri, Mar 25, 2022 at 10:09:33PM +0100, Daniel Gustafsson wrote:
>> > Agreed.  In this case it seems that adding --exclude-extension would
>> make sense
>> > to keep conistency.  I took a quick stab at doing so with the attached
>> while
>> > we're here.
>>
>> src/test/modules/test_pg_dump would be the best place for the addition
>> of a couple of tests with this new switch.  Better to check as well
>> what happens when a command collides with --extension and
>> --exclude-extension.
>>
>> printf(_("  -e, --extension=PATTERN  dump the specified
>> extension(s) only\n"));
>> +   printf(_("  --exclude-extension=PATTERN  do NOT dump the specified
>> extension(s)\n"));
>> Shouldn't this be listed closer to --exclude-table-data in the --help
>> output?
>>
>
> I think it's time to sum up what we want to do:
>
> - We'd like to use switches to export objects according to a pattern.
> - For each object type we will have an --object=PATTERN flag and a
> --exclude-object=PATTERN
> - Having a short flag for each of the long flags is not mandatory
> - The object types that pg_dump can select so far are:
> - table (already written)
> - schema (already written)
> - extension (half-written, --exclude-extension not written)
> - routine (TBD ASAP). Routine flag operates on stored functions,
> stored procedures, aggregate functions, and window functions.
> - By default, pg_dump does not export system objects but we found out that
> we could use --table='pg_catalog.*' to export them. This is a bug and will
> be fixed. pg_dump won't have the ability to export any system object
> anymore. Should the fix belong to that patch or do I need to create a
> separate patch? (Seems to me it should be separated)
>
> If everyone is ok with the points above, I'll write both patches.
>
>
That looks correct.

I would say we should make the --table change and the --exclude-extension
change as separate commits.

Michael's question brought up a point that we should address.  I do not
think having these (now) 4 pairs of options presented strictly
alphabetically in the documentation is a good choice and we should deviate
from that convention here for something more user-friendly, and to reduce
the repetitiveness that comes from having basically what could be one pair
of options actually implemented as 3 pairs.  My initial approach would be
to move them all to a subsection after the --help parameter and before the
section header for -d.  That section would be presented something like:

"""
These options allow for fine-grained control of which user objects are
produced by the dump (system objects are never dumped).  If no inclusion
options are specified all objects are dumped except those that are
explicitly excluded.  If even one inclusion option is specified then only
those objects selected for inclusion, and not excluded, will appear in the
dump.

These options can appear multiple times within a single pg_dump command
line. For each of these there is a mandatory pattern value, so the actual
option looks like, e.g., --table='public.*', which will select all
relations in the public schema. See (Patterns). When using wildcards, be
careful to quote the pattern if needed to prevent the shell from expanding
the wildcards.

When using these options, dependencies of the selected objects are not
automatically dumped, thus making such a dump potentially unsuitable for
restoration into a clean database.

This subset of options control which schemas to select objects from for an
otherwise normal dump.

--schema / -n
--exclude-schema / -N

The following subset specifies which non-schema objects to include.  These
are added to the objects that end up selected due to their presence in a
schema.  Specifically, the --exclude-schema option is ignored while
processing these options.

--table / -t
  Considers all relations, not just tables.  i.e., views, materialized
views, foreign tables, and sequences.
--routine
  Considers functions, procedures, aggregates, window functions
--extension / -e
  Considers extensions

When dumping data, only local table data is dumped by default.  Specific
table data can be excluded using the --exclude-table-data option.

Specifying a foreign server using --include-foreign-data will cause related
foreign table data to also be dumped.

The following subset specifies which objects to exclude.  An object that
matches one of these patterns will never be dumped.

--exclude-table / -T
--exclude-routine
--exclude-extension

The following options control the dumping of large objects:

-b
--blobs
Include large objects in the dump. This is the default behavior except when
--schema, --table, or --schema-only is specified. The -b switch is
therefore only useful to add large objects to dumps where a specific schema
or table has been requested. Note that blobs are considered data and

Re: Pointer subtraction with a null pointer

[Tom Lane]

Andres Freund  writes:
> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
>> This code is old, but mylodon wasn't doing that a week ago, so
>> Andres must've updated the compiler and/or changed its options.

> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

OK, that answers that.

After more thought I agree that replacing these relptr_store calls
with something else would be the better solution.  I'll prepare a
patch.

regards, tom lane

Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors

[Tom Lane]

Tatsuo Ishii  writes:
> Thanks. Patch pushed.

This patch has caused the PDF documentation to fail to build cleanly:

[WARN] FOUserAgent - The contents of fo:block line 1 exceed the available area 
in the inline-progression direction by more than 50 points. (See position 
125066:375)

It's complaining about this:


interval_start 
num_transactions 
sum_latency sum_latency_2 
min_latency max_latency { 
failures | 
serialization_failures 
deadlock_failures }  
sum_lag sum_lag_2 
min_lag max_lag 
 skipped   
 retried 
retries 


which runs much too wide in HTML format too, even though that toolchain
doesn't tell you so.

We could silence the warning by inserting an arbitrary line break or two,
or refactoring the syntax description into multiple parts.  Either way
seems to create a risk of confusion.

TBH, I think the *real* problem is that the complexity of this log format
has blown past "out of hand".  Can't we simplify it?  Who is really going
to use all these numbers?  I pity the poor sucker who tries to write a
log analysis tool that will handle all the variants.

regards, tom lane

Re: Pointer subtraction with a null pointer

[Andres Freund]

Hi,

On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
> This code is old, but mylodon wasn't doing that a week ago, so
> Andres must've updated the compiler and/or changed its options.

Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

I'll update it to 14 soon, now that that's released. It still has that
warning, so it's not going to help us avoid the warning.

Greetings,

Andres Freund

Re: Pointer subtraction with a null pointer

[Isaac Morland]

On Sat, 26 Mar 2022 at 12:24, Andres Freund  wrote:


> NULL can never be part of the same "array object" or one past past the last
> element as the pointer it is subtracted from. Hence the undefined beaviour.
>

Even more fundamentally, NULL is not 0 in any ordinary mathematical sense,
even though it can be written 0 in source code and is often (but not
always) represented in memory as an all-0s bit pattern. I'm not at all
surprised to learn that arithmetic involving NULL is undefined.

Re: Pointer subtraction with a null pointer

[Andres Freund]

Hi,

On 2022-03-26 12:04:54 -0400, Tom Lane wrote:
> Several of Andres' buildfarm animals have recently started to whine
> that "performing pointer subtraction with a null pointer has undefined
> behavior" for assorted places in freepage.c.
>
> From a mathematical standpoint, this astonishes me: "x - 0 = x" is a
> tautology.

I don't think that's quite what the warning is warning about. The C standard
doesn't allow pointer arithmetic between arbitrary pointers, they have to be
to the same "object" (plus a trailing array element).

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf 6.5.6 Additive
operators, 8/9

  When two pointers are subtracted, both shall point to elements of the same 
array object,
  or one past the last element of the array object; the result is the 
difference of the
  subscripts of the two array elements.

NULL can never be part of the same "array object" or one past past the last
element as the pointer it is subtracted from. Hence the undefined beaviour.


> Or maybe we should change these call sites to do something different,
> because this is surely abusing the intent of relptr_store.

I think a relptr_zero(), relptr_setnull() or such would make sense. That'd get
rid of the need for the cast as well.

Greetings,

Andres Freund

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

LZ4F_HEADER_SIZE_MAX isn't defined in old LZ4.

I ran into that on an ubuntu LTS, so I don't think it's so old that it
shouldn't be handled more gracefully.  LZ4 should either have an explicit
version check, or else shouldn't depend on that feature (or should define a
safe fallback version if the library header doesn't define it).

https://packages.ubuntu.com/liblz4-1

0003: typo: of legacy => or legacy

There are a large number of ifdefs being added here - it'd be nice to minimize
that.  basebackup was organized to use separate files, which is one way.

$ git grep -c 'ifdef .*LZ4' src/bin/pg_dump/compress_io.c
src/bin/pg_dump/compress_io.c:19

In last year's CF entry, I had made a union within CompressorState.  LZ4
doesn't need z_streamp (and ztsd will need ZSTD_outBuffer, ZSTD_inBuffer,
ZSTD_CStream).

0002: I wonder if you're able to re-use any of the basebackup parsing stuff
from commit ffd53659c.  You're passing both the compression method *and* level.
I think there should be a structure which includes both.  In the future, that
can also handle additional options.  I hope to re-use these same things for
wal_compression=method:level.

You renamed this:

|-   COMPR_ALG_LIBZ
|-} CompressionAlgorithm;
|+   COMPRESSION_GZIP,
|+} CompressionMethod;

..But I don't think that's an improvement.  If you were to change it, it should
say something like PGDUMP_COMPRESS_ZLIB, since there are other compression
structs and typedefs.  zlib is not idential to gzip, which uses a different
header, so in WriteDataToArchive(), LIBZ is correct, and GZIP is incorrect.

The cf* changes in pg_backup_archiver could be split out into a separate
commit.  It's strictly a code simplification - not just preparation for more
compression algorithms.  The commit message should "See also:
bf9aa490db24b2334b3595ee33653bf2fe39208c".

The changes in 0002 for cfopen_write seem insufficient:
|+   if (compressionMethod == COMPRESSION_NONE)
|+   fp = cfopen(path, mode, compressionMethod, 0);
|else
|{
| #ifdef HAVE_LIBZ
|char   *fname;
| 
|fname = psprintf("%s.gz", path);
|-   fp = cfopen(fname, mode, compression);
|+   fp = cfopen(fname, mode, compressionMethod, compressionLevel);
|free_keep_errno(fname);
| #else

The only difference between the LIBZ and uncompressed case is the file
extension, and it'll be the only difference with LZ4 too.  So I suggest to
first handle the file extension, and the rest of the code path is not
conditional on the compression method.  I don't think cfopen_write even needs
HAVE_LIBZ - can't you handle that in cfopen_internal() ?

This patch rejects -Z0, which ought to be accepted:
./src/bin/pg_dump/pg_dump -h /tmp regression -Fc -Z0 |wc
pg_dump: error: can only specify -Z/--compress [LEVEL] when method is set

Your 0003 patch shouldn't reference LZ4:
+#ifndef HAVE_LIBLZ4
+   if (*compressionMethod == COMPRESSION_LZ4)
+   supports_compression = false;
+#endif

The 0004 patch renames zlibOutSize to outsize - I think the patch series should
be constructed such as to minimize the size of the method-specific patches.  I
say this anticipating also adding support for zstd.  The preliminary patches
should have all the boring stuff.  It would help for reviewing to keep the 
patches split up, or to enumerate all the boring things that are being renamed
(like change OutputContext to cfp, rename zlibOutSize, ...).

0004: The include should use  and not "lz4.h"

freebsd/cfbot is failing.

I suggested off-list to add an 0099 patch to change LZ4 to the default, to
exercise it more on CI.

-- 
Justin

Re: Pointer subtraction with a null pointer

[Tom Lane]

I wrote:
> Several of Andres' buildfarm animals have recently started to whine
> that "performing pointer subtraction with a null pointer has undefined
> behavior" for assorted places in freepage.c.

Ah, sorry, I meant to include a link:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=mylodon=2022-03-26%2000%3A02%3A10=make

This code is old, but mylodon wasn't doing that a week ago, so
Andres must've updated the compiler and/or changed its options.
kestrel and olingo are reporting it too, but they're new.

regards, tom lane

Re: Invalid comment in ParallelQueryMain

[Matthias van de Meent]

On Sat, 26 Mar 2022 at 17:01, Julien Rouhaud  wrote:
>
> Hi,
>
> While reading code around I just noticed that I failed to adapt a comment a
> couple of lines above a removed line in 0f61727b75b9.  Patch attached.

+1, seems OK to me.

Pointer subtraction with a null pointer

[Tom Lane]

Several of Andres' buildfarm animals have recently started to whine
that "performing pointer subtraction with a null pointer has undefined
behavior" for assorted places in freepage.c.

>From a mathematical standpoint, this astonishes me: "x - 0 = x" is a
tautology.  So I'm a bit inclined to say "you're full of it" and disable
-Wnull-pointer-subtraction.  On the other hand, all of the occurrences
are in calls of relptr_store with a constant-NULL third argument.
So we could silence them without too much pain by adjusting that macro
to special-case NULL.  Or maybe we should change these call sites to do
something different, because this is surely abusing the intent of
relptr_store.

Thoughts?

regards, tom lane

Invalid comment in ParallelQueryMain

[Julien Rouhaud]

Hi,

While reading code around I just noticed that I failed to adapt a comment a
couple of lines above a removed line in 0f61727b75b9.  Patch attached.
>From 5ba34dd1129cd1e2038ac70992a6b5e77df6121e Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Sat, 26 Mar 2022 23:55:59 +0800
Subject: [PATCH v1] Fix comment in ParallelQueryMain

Oversight in 0f61727b75b93915ca9a9f20c996ed7020996a38.

Backpatch to 14.
---
 src/backend/executor/execParallel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/executor/execParallel.c 
b/src/backend/executor/execParallel.c
index 5dd8ab7db2..9a0d5d59ef 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -1420,7 +1420,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
/* Setting debug_query_string for individual workers */
debug_query_string = queryDesc->sourceText;
 
-   /* Report workers' query and queryId for monitoring purposes */
+   /* Report workers' query for monitoring purposes */
pgstat_report_activity(STATE_RUNNING, debug_query_string);
 
/* Attach to the dynamic shared memory area. */
-- 
2.33.1

Re: psql - add SHOW_ALL_RESULTS option

[Fabien COELHO]

Hello Peter,

As Tom said earlier, wasn't this fixed by 618c16707?  If not, is there any 
other discussion on the specifics of this issue?  I'm not aware of one.


This answer is that I had kept psql's calls to PQerrorMessage which 
reports errors from the connection, whereas it needed to change to 
PQresultErrorMessage to benefit from the libpq improvement.


I made the added autocommit/on_error_rollback tests at the end really 
focus on multi-statement queries (\;), as was more or less intended.


I updated the tap test.

--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
 SELECT 2.0 AS "float" \;
 SELECT 'world' AS "text" \;
 COMMIT;
+ float 
+---
+   2.0
+(1 row)
+
+ text  
+---
+ world
+(1 row)
+
 -- compound with empty statements and spurious leading spacing
 \;\;   SELECT 3 + 3 \;\;\;   SELECT ' ' || ' !' \;\;   SELECT 1 + 4 \;;
+ ?column? 
+--
+6
+(1 row)
+
+ ?column? 
+--
+   !
+(1 row)
+
  ?column? 
 --
 5
@@ -61,6 +81,11 @@ COMMIT;
 SELECT 1 + 1 + 1 AS "add" \gset
 SELECT :add + 1 + 1 AS "add" \;
 SELECT :add + 1 + 1 AS "add" \gset
+ add 
+-
+   5
+(1 row)
+
 -- set operator
 SELECT 1 AS i UNION SELECT 2 ORDER BY i;
  i 
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions.  (See 
for more details about how the server handles multi-query strings.)
-   Also, psql only prints the
-   result of the last SQL command in the string.
-   This is different from the behavior when the same string is read from
-   a file or fed to psql's standard input,
-   because then psql sends
-   each SQL command separately.
   
   
-   Because of this behavior, putting more than one SQL command in a
-   single -c string often has unexpected results.
-   It's better to use repeated -c commands or feed
-   multiple commands to psql's standard input,
+   If having several commands executed in one transaction is not desired, 
+   use repeated -c commands or feed multiple commands to
+   psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
 
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
 commands included in the string to divide it into multiple
 transactions.  (See 
 for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
 
 
   
@@ -4160,6 +4149,18 @@ bar
   
 
   
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them.  The default is on.  The off behavior
+is for compatibility with older versions of psql.
+
+
+  
+
+   
 SHOW_CONTEXT
 
 
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..c84688e89c 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
 static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
 static bool command_no_begin(const char *query);
 static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+	bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
 
 
 /*
@@ -354,7 +356,7 @@ CheckConnection(void)
  * Returns true for valid result, false for error state.
  */
 static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
 {
 	bool		OK;
 
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
 break;
 		}
 
-	if (!OK)
+	if (!OK && show_error)
 	{
 		const char *error = PQerrorMessage(pset.db);
 
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
 	}
 }
 
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+	PGresult	*result;
+
+	while ((result = PQgetResult(pset.db)) != NULL)
+		ClearOrSaveResult(result);
+}
+
 
 /*
  * Print microtiming output.  Always print raw milliseconds; if the interval
@@ -573,7 +587,7 @@ PSQLexec(const char *query)
 
 	ResetCancelConn();
 
-	if (!AcceptResult(res))
+	if 

PostgreSQL 15 Release Management Team (RMT) + Feature Freeze

[Jonathan S. Katz]

Hi,

We are pleased to announce the Release Management Team (RMT) (cc'd) for 
the PostgreSQL 15 release:


  - John Naylor
  - Jonathan Katz
  - Michael Paquier

You can find information about the responsibilities of the RMT here:

  https://wiki.postgresql.org/wiki/Release_Management_Team

Additionally, the RMT has set the feature freeze date to be April 7, 
2022. This is the last day to commit features for PostgreSQL 15. In 
other words, no new PostgreSQL 15 feature can be committed after April 8 
0:00, 2022 AoE[1].


You can track open items for the PostgreSQL 15 release here:

  https://wiki.postgresql.org/wiki/PostgreSQL_15_Open_Items

Please let us know if you have any questions.

On behalf of the PG15 RMT,

Jonathan

[1] https://en.wikipedia.org/wiki/Anywhere_on_Earth


OpenPGP_signature
Description: OpenPGP digital signature

Re: Remove an unused function GetWalRcvWriteRecPtr

[Tom Lane]

Julien Rouhaud  writes:
> On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
>> This could be used by some external module, no?

> Maybe, but WalRcv is exposed so if an external module needs it it could still
> maintain its own version of GetWalRcvWriteRecPtr.

We'd need to mark WalRcv as PGDLLIMPORT if we want to take that
seriously.

regards, tom lane

Re: Identify missing publications from publisher while create/alter subscription.

[vignesh C]

On Tue, Mar 22, 2022 at 3:23 PM vignesh C  wrote:
>
> On Tue, Mar 22, 2022 at 5:29 AM Andres Freund  wrote:
> >
> > On 2022-02-13 19:34:05 +0530, vignesh C wrote:
> > > Thanks for the comments, the attached v14 patch has the changes for the 
> > > same.
> >
> > The patch needs a rebase, it currently fails to apply:
> > http://cfbot.cputube.org/patch_37_2957.log

The patch was not applying on HEAD, attached patch which is rebased on
top of HEAD.

Regards,
Vignesh
From 1c4096fc1c2b3cfa54d225c185a9e85bab114d8a Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Sat, 26 Mar 2022 19:43:27 +0530
Subject: [PATCH v15] Identify missing publications from publisher while
 create/alter subscription.

Creating/altering subscription is successful when we specify a publication which
does not exist in the publisher. This patch checks if the specified publications
are present in the publisher and throws an error if any of the publication is
missing in the publisher.

Author: Vignesh C
Reviewed-by: Bharath Rupireddy, Japin Li, Dilip Kumar, Euler Taveira, Ashutosh Sharma
Discussion: https://www.postgresql.org/message-id/flat/20220321235957.i4jtjn4wyjucex6b%40alap3.anarazel.de#b846fd4ef657cfaa8c9890f044e4
---
 doc/src/sgml/ref/alter_subscription.sgml  |  13 ++
 doc/src/sgml/ref/create_subscription.sgml |  20 ++-
 src/backend/commands/subscriptioncmds.c   | 201 +++---
 src/bin/psql/tab-complete.c   |  15 +-
 src/test/subscription/t/007_ddl.pl|  54 ++
 5 files changed, 270 insertions(+), 33 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 3e46bbdb04..9f6a029600 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -172,6 +172,19 @@ ALTER SUBSCRIPTION name RENAME TO <
  
 

+
+   
+validate_publication (boolean)
+
+ 
+  When true, the command verifies if all the specified publications
+  that are being subscribed to are present in the publisher and throws
+  an error if any of the publications doesn't exist. The default is
+  false.
+ 
+
+   
+
   
 

diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index b701752fc9..f2e7e8744d 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -110,12 +110,14 @@ CREATE SUBSCRIPTION subscription_nametrue.  Setting this to
   false will force the values of
-  create_slot, enabled and
-  copy_data to false.
+  create_slot, enabled,
+  copy_data and
+  validate_publication to false.
   (You cannot combine setting connect
   to false with
   setting create_slot, enabled,
-  or copy_data to true.)
+  copy_data or
+  validate_publication to true.)
  
 
  
@@ -170,6 +172,18 @@ CREATE SUBSCRIPTION subscription_name
 

+
+   
+validate_publication (boolean)
+
+ 
+  When true, the command verifies if all the specified publications
+  that are being subscribed to are present in the publisher and throws
+  an error if any of the publications doesn't exist. The default is
+  false.
+ 
+
+   
   
  
 
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index abebffdf3b..ba6c2693d4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -64,6 +64,7 @@
 #define SUBOPT_TWOPHASE_COMMIT		0x0200
 #define SUBOPT_DISABLE_ON_ERR		0x0400
 #define SUBOPT_LSN	0x0800
+#define SUBOPT_VALIDATE_PUB			0x1000
 
 /* check if the 'val' has 'bits' set */
 #define IsSet(val, bits)  (((val) & (bits)) == (bits))
@@ -86,6 +87,7 @@ typedef struct SubOpts
 	bool		streaming;
 	bool		twophase;
 	bool		disableonerr;
+	bool		validate_publication;
 	XLogRecPtr	lsn;
 } SubOpts;
 
@@ -138,6 +140,8 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 		opts->twophase = false;
 	if (IsSet(supported_opts, SUBOPT_DISABLE_ON_ERR))
 		opts->disableonerr = false;
+	if (IsSet(supported_opts, SUBOPT_VALIDATE_PUB))
+		opts->validate_publication = false;
 
 	/* Parse options */
 	foreach(lc, stmt_options)
@@ -293,6 +297,15 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 			opts->specified_opts |= SUBOPT_LSN;
 			opts->lsn = lsn;
 		}
+		else if (IsSet(supported_opts, SUBOPT_VALIDATE_PUB) &&
+ strcmp(defel->defname, "validate_publication") == 0)
+		{
+			if (IsSet(opts->specified_opts, SUBOPT_VALIDATE_PUB))
+errorConflictingDefElem(defel, pstate);
+
+			opts->specified_opts |= SUBOPT_VALIDATE_PUB;
+			opts->validate_publication = defGetBoolean(defel);
+		}
 		else
 			ereport(ERROR,
 	

Re: pg_relation_size on partitioned table

[Bharath Rupireddy]

On Sat, Mar 26, 2022 at 11:35 AM Michael Paquier  wrote:
>
> On Fri, Mar 25, 2022 at 08:52:40PM +0800, Japin Li wrote:
> > Could we provide a function to get the total size of the partition table
> > though the partitioned table name?  Maybe we can extend
> > the pg_relation_size() to get the total size of partition tables through
> > the partitioned table name.
>
> There are already many replies on this thread, but nobody has
> mentioned pg_partition_tree() yet, so here you go.  You could use that
> in combination with pg_relation_size() to get the whole size of a tree
> depending on your needs.

Yeah. The docs have a note on using it for finding partitioned table size:

   
For example, to check the total size of the data contained in a
partitioned table measurement, one could use the
following query:

SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size
  FROM pg_partition_tree('measurement');

   

Regards,
Bharath Rupireddy.

Re: Remove an unused function GetWalRcvWriteRecPtr

[Bharath Rupireddy]

On Sat, Mar 26, 2022 at 12:55 PM Julien Rouhaud  wrote:
>
> On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
> > On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote:
> > > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > > pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without
> > > spinlock) is being used directly in pg_stat_get_wal_receiver
> > > walreceiver.c. We either make use of the function instead of
> > > pg_atomic_read_u64(>writtenUpto); or remove it. Since there's
> > > only one function using walrcv->writtenUpto right now, I prefer to
> > > remove the function to save some LOC (13).
> > >
> > > Attaching patch. Thoughts?
> >
> > This could be used by some external module, no?
>
> Maybe, but WalRcv is exposed so if an external module needs it it could still
> maintain its own version of GetWalRcvWriteRecPtr.

Yes. And the core extensions aren't using GetWalRcvWriteRecPtr. IMO,
let's not maintain that function.

Regards,
Bharath Rupireddy.

Re: Skipping schema changes in publication

[vignesh C]

On Tue, Mar 22, 2022 at 12:38 PM vignesh C  wrote:
>
> Hi,
>
> This feature adds an option to skip changes of all tables in specified
> schema while creating publication.
> This feature is helpful for use cases where the user wants to
> subscribe to all the changes except for the changes present in a few
> schemas.
> Ex:
> CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> OR
> ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
>
> A new column pnskip is added to table "pg_publication_namespace", to
> maintain the schemas that the user wants to skip publishing through
> the publication. Modified the output plugin (pgoutput) to skip
> publishing the changes if the relation is part of skip schema
> publication.
> As a continuation to this, I will work on implementing skipping tables
> from all tables in schema and skipping tables from all tables
> publication.
>
> Attached patch has the implementation for this.

The patch was not applying on top of HEAD because of the recent
commits, attached patch is rebased on top of HEAD.

Regards,
Vignesh
From 1524254629c72149ae99d13aae283d3aa6a70253 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Sat, 26 Mar 2022 19:19:31 +0530
Subject: [PATCH v1] Skip publishing the tables of schema.

A new option "SKIP ALL TABLES IN SCHEMA" in Create/Alter Publication allows
one or more skip schemas to be specified, publisher will skip sending the data
of the tables present in the skip schema to the subscriber.

The new syntax allows specifying schemas. For example:
CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
OR
ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;

A new column pnskip is added to table "pg_publication_namespace", to maintain
the schemas that the user wants to skip publishing through the publication.
Modified the output plugin (pgoutput) to skip publishing the changes if the
relation is part of skip schema publication.

Updates pg_dump to identify and dump skip schema publications. Updates the \d
family of commands to display skip schema publications and \dRp+ variant will
now display associated skip schemas if any.
---
 doc/src/sgml/catalogs.sgml|   9 +
 doc/src/sgml/logical-replication.sgml |   7 +-
 doc/src/sgml/ref/alter_publication.sgml   |  28 ++-
 doc/src/sgml/ref/create_publication.sgml  |  28 ++-
 doc/src/sgml/ref/psql-ref.sgml|   5 +-
 src/backend/catalog/pg_publication.c  |  70 +--
 src/backend/commands/publicationcmds.c| 186 +++---
 src/backend/commands/tablecmds.c  |   6 +-
 src/backend/nodes/copyfuncs.c |  14 ++
 src/backend/nodes/equalfuncs.c|  14 ++
 src/backend/parser/gram.y | 119 ++-
 src/backend/replication/pgoutput/pgoutput.c   |  25 +--
 src/backend/utils/cache/relcache.c|  21 +-
 src/bin/pg_dump/pg_dump.c |  33 +++-
 src/bin/pg_dump/pg_dump.h |   1 +
 src/bin/pg_dump/pg_dump_sort.c|   7 +
 src/bin/pg_dump/t/002_pg_dump.pl  |  30 +++
 src/bin/psql/describe.c   |  19 +-
 src/bin/psql/tab-complete.c   |  26 ++-
 src/include/catalog/pg_publication.h  |  20 +-
 .../catalog/pg_publication_namespace.h|   1 +
 src/include/commands/publicationcmds.h|   6 +-
 src/include/nodes/nodes.h |   1 +
 src/include/nodes/parsenodes.h|   1 +
 src/test/regress/expected/publication.out |  81 +++-
 src/test/regress/sql/publication.sql  |  41 +++-
 .../t/032_rep_changes_skip_schema.pl  |  96 +
 src/tools/pgindent/typedefs.list  |   1 +
 28 files changed, 742 insertions(+), 154 deletions(-)
 create mode 100644 src/test/subscription/t/032_rep_changes_skip_schema.pl

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 560e205b95..59b0126c18 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6304,6 +6304,15 @@ SCRAM-SHA-256$iteration count:
A null value indicates that all columns are published.
   
  
+
+
+  
+   pnskip bool
+  
+  
+   True if the schema is skip schema
+  
+ 
 

   
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index 555fbd749c..e2a4b89226 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -599,9 +599,10 @@ CONTEXT:  processing remote data for replication origin "pg_16395" during "INSER
 
   
To add tables to a publication, the user must have ownership rights on the
-   table. To add all tables in schema to a publication, the user must be a
-   superuser. To create a publication that publishes all tables or all tables in
-   schema automatically, the user must be a superuser.
+   table. To add all tables in schema or 

Re: SQL/JSON: JSON_TABLE

[Andrew Dunstan]

On 3/26/22 07:29, Erik Rijkers wrote:
> Op 25-03-2022 om 21:30 schreef Andrew Dunstan:
>>
>> On 3/22/22 10:55, Daniel Gustafsson wrote:
 On 22 Mar 2022, at 16:31, Andrew Dunstan  wrote:
 I'm planning on pushing the functions patch set this week and
 json-table
 next week.
>>> My comments from 30827b3c-edf6-4d41-bbf1-298181874...@yesql.se are
>>> yet to be
>>> addressed (or at all responded to) in this patchset.  I'll paste the
>>> ones which
>>> still apply to make it easier:
>>>
>>
>>
>> I think I have fixed all those. See attached. I haven't prepared a new
>> patch set for SQL/JSON functions because there's just one typo to fix,
>> but I won't forget it. Please let me know if there's anything else
>> you see.
>>
>>
>> At this stage I think I have finished with the actual code, and I'm
>> concentrating on improving the docs a bit.
>
> > [ v59 ]
>
>
> FWIW, I went through func.sgml (of v59) once.
>
>
>


Thanks, That will help.


cheers


andrew


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

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Fri, Mar 25, 2022 at 8:16 PM Dilip Kumar  wrote:
>
> > On a quick look, I'm guessing that XLOG_DBASE_CREATE_WAL_LOG will need
> > to mirror some of the logic that was added to the replay code for the
> > existing strategy, but I haven't figured out the details.
> >
>
> Yeah, I think I got it, for XLOG_DBASE_CREATE_WAL_LOG now we will have
> to handle the missing parent directory case, like Alvaro handled for
> the XLOG_DBASE_CREATE(_FILE_COPY) case.

I have updated the patch so now we skip the XLOG_DBASE_CREATE_WAL_LOG
as well if the tablespace directory is missing.  But with our new
wal_log method there will be other follow up wal logs like,
XLOG_RELMAP_UPDATE, XLOG_SMGR_CREATE and XLOG_FPI.

I have put the similar logic for relmap_update WAL replay as well, but
we don't need this  for smgr_create or fpi.  Because the mdcreate() is
taking care of creating missing directory in TablespaceCreateDbspace()
and fpi only logged after we create the new smgr at least in case of
create database.

Now, is it possible to get the FPI without smgr_create wal in other
cases?  If it is then that problem is orthogonal to this path, but
anyway I could not find any such scenario.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From e0133aa89ca5da8309d07e4236ded4af513d3905 Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Sat, 26 Mar 2022 10:35:44 +0530
Subject: [PATCH v7] Add new block-by-block strategy for CREATE DATABASE.

Because this strategy logs changes on a block-by-block basis, it
avoids the need to checkpoint before and after the operation.
However, because it logs each changed block individually, it might
generate a lot of extra write-ahead logging if the template database
is large. Therefore, the older strategy remains available via a new
STRATEGY parameter to CREATE DATABASE, and a corresponding --strategy
option to createdb.

Somewhat controversially, this patch assembles the list of relations
to be copied to the new database by reading the pg_class relation of
the template database. Cross-database access like this isn't normally
possible, but it can be made to work here because there can't be any
connections to the database being copied, nor can it contain any
in-doubt transactions. Even so, we have to use lower-level interfaces
than normal, since the table scan and relcache interfaces will not
work for a database to which we're not connected. The advantage of
this approach is that we do not need to rely on the filesystem to
determine what ought to be copied, but instead on PostgreSQL's own
knowledge of the database structure. This avoids, for example,
copying stray files that happen to be located in the source database
directory.

Dilip Kumar, with a fairly large number of cosmetic changes by me.
---
 contrib/bloom/blinsert.c |   2 +-
 doc/src/sgml/monitoring.sgml |   4 +
 doc/src/sgml/ref/create_database.sgml|  22 +
 doc/src/sgml/ref/createdb.sgml   |  11 +
 src/backend/access/heap/heapam_handler.c |   6 +-
 src/backend/access/nbtree/nbtree.c   |   2 +-
 src/backend/access/rmgrdesc/dbasedesc.c  |  20 +-
 src/backend/access/transam/xlogutils.c   |   6 +-
 src/backend/catalog/heap.c   |   2 +-
 src/backend/catalog/storage.c|  34 +-
 src/backend/commands/dbcommands.c| 795 ++-
 src/backend/commands/tablecmds.c |   2 +-
 src/backend/storage/buffer/bufmgr.c  | 172 ++-
 src/backend/storage/lmgr/lmgr.c  |  28 ++
 src/backend/utils/activity/wait_event.c  |   3 +
 src/backend/utils/cache/relcache.c   |   2 +-
 src/backend/utils/cache/relmapper.c  |  97 
 src/bin/pg_rewind/parsexlog.c|   9 +-
 src/bin/psql/tab-complete.c  |   4 +-
 src/bin/scripts/createdb.c   |  10 +-
 src/bin/scripts/t/020_createdb.pl|  20 +
 src/include/catalog/storage.h|   4 +-
 src/include/commands/dbcommands_xlog.h   |  25 +-
 src/include/storage/bufmgr.h |   6 +-
 src/include/storage/lmgr.h   |   1 +
 src/include/utils/relmapper.h|   4 +-
 src/include/utils/wait_event.h   |   1 +
 src/tools/pgindent/typedefs.list |   5 +-
 28 files changed, 1140 insertions(+), 157 deletions(-)

diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index c94cf34..82378db 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -173,7 +173,7 @@ blbuildempty(Relation index)
 	 * Write the page and log it.  It might seem that an immediate sync would
 	 * be sufficient to guarantee that the file exists on disk, but recovery
 	 * itself might remove it while replaying, for example, an
-	 * XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record.  Therefore, we need
+	 * XLOG_DBASE_CREATE* or XLOG_TBLSPC_CREATE record.  Therefore, we need
 	 * this even when wal_level=minimal.
 	 */
 	PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
diff --git a/doc/src/sgml/monitoring.sgml 

Re: SQL/JSON: JSON_TABLE

[Erik Rijkers]

Op 25-03-2022 om 21:30 schreef Andrew Dunstan:


On 3/22/22 10:55, Daniel Gustafsson wrote:

On 22 Mar 2022, at 16:31, Andrew Dunstan  wrote:
I'm planning on pushing the functions patch set this week and json-table
next week.

My comments from 30827b3c-edf6-4d41-bbf1-298181874...@yesql.se are yet to be
addressed (or at all responded to) in this patchset.  I'll paste the ones which
still apply to make it easier:




I think I have fixed all those. See attached. I haven't prepared a new
patch set for SQL/JSON functions because there's just one typo to fix,
but I won't forget it. Please let me know if there's anything else you see.


At this stage I think I have finished with the actual code, and I'm
concentrating on improving the docs a bit.


> [ v59 ]


FWIW, I went through func.sgml (of v59) once.


Erik Rijkers
--- doc/src/sgml/func.sgml.orig	2022-03-25 22:17:13.908660140 +0100
+++ doc/src/sgml/func.sgml	2022-03-26 12:08:46.593271826 +0100
@@ -17673,8 +17673,8 @@
  Description
 
  
-  JSON function generates a JSON
-  from a text data.
+  The JSON function generates JSON
+  from text data.
  
 
 
@@ -17688,7 +17688,7 @@
 
  
   String expression that provides the JSON text data.
-  Accepted any character strings (text, char, etc.)
+  Accepts any character strings (text, char, etc.)
   or binary strings (bytea) in UTF8 encoding.
   For null input, SQL null value is returned.
  
@@ -17757,7 +17757,7 @@
 
  Examples
  
-  Construct a JSON the provided strings:
+  Construct JSON using the provided strings:
  
 
 SELECT JSON('{ "a" : 123, "b": [ true, "foo" ], "a" : "bar" }');
@@ -17794,8 +17794,8 @@
  Description
 
  
-  JSON_SCALAR function generates a scalar
-  JSON from a SQL data.
+  The JSON_SCALAR function generates scalar
+  JSON from SQL data.
  
 
 
@@ -17808,11 +17808,11 @@
 
 
  
-  Expression that provides the data for constructing a
+  Expression that provides the data for constructing
   JSON.
   For null input, SQL  null
-  (not a JSON null) value is returned.
-  For any scalar other than a number, a Boolean, the text representation
+  (not JSON null) value is returned.
+  For any scalar other than a number or a Boolean, the text representation
   will be used, with escaping as necessary to make it a valid
   JSON string value.
   For details, see
@@ -17847,7 +17847,7 @@
 
  Examples
  
-  Construct a JSON from the provided values various types:
+  Construct JSON from provided values of various type:
  
 
 SELECT JSON_SCALAR(123.45);
@@ -18753,7 +18753,7 @@
 
 -- Strict mode with ERROR on ERROR clause
 SELECT JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' ERROR ON ERROR);
-ERROR: Invalid SQL/JSON subscript
+ERROR:  jsonpath array subscript is out of bounds
 (1 row)
 
 
@@ -18795,11 +18795,11 @@
  Description
 
   
-   JSON_VALUE function extracts a value from the provided
+   The JSON_VALUE function extracts a value from the provided
JSON data and converts it to an SQL scalar.
If the specified JSON path expression returns more than one
SQL/JSON item, an error occurs. To extract
-   an SQL/JSON array or object, use .
+   an SQL/JSON array or object, see .
   
 
 
@@ -18885,19 +18885,19 @@
  
 
 
-SELECT JSON_VALUE('"123.45"', '$' RETURNING float);
+SELECT JSON_VALUE('"123.45"'::jsonb, '$' RETURNING float);
  json_value
 
  123.45
 (1 row)
 
-SELECT JSON_VALUE('123.45', '$' RETURNING int ERROR ON ERROR);
+SELECT JSON_VALUE('123.45'::jsonb, '$' RETURNING int ERROR ON ERROR);
  json_value
 
 123
 (1 row)
 
-SELECT JSON_VALUE('"03:04 2015-02-01"', '$.datetime("HH24:MI -MM-DD")' RETURNING date);
+SELECT JSON_VALUE('"03:04 2015-02-01"'::jsonb, '$.datetime("HH24:MI -MM-DD")' RETURNING date);
  json_value 
 
  2015-02-01
@@ -18907,10 +18907,10 @@
 ERROR:  invalid input syntax for integer: "123.45"
 
 SELECT JSON_VALUE(jsonb '[1]', 'strict $' ERROR ON ERROR);
-ERROR: SQL/JSON scalar required
+ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item
 
 SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]' ERROR ON ERROR);
-ERROR: more than one SQL/JSON item
+ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item
 
 
  
@@ -18920,13 +18920,13 @@
  
 
 SELECT JSON_VALUE(jsonb '[1]', 'strict $' ERROR ON ERROR);
-ERROR: SQL/JSON scalar required
+ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item
 
 SELECT JSON_VALUE(jsonb '{"a": 1}', 'strict $' ERROR ON ERROR);
-ERROR: SQL/JSON scalar required
+ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item
 
 SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]' ERROR ON ERROR);
-ERROR: more than one 

Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

[Michael Paquier]

On Wed, Mar 23, 2022 at 03:17:35PM +0900, Michael Paquier wrote:
> FWIW, per my review the bit of the patch set that I found the most
> relevant is the addition of a note in the docs of pg_stat_file() about
> the case where "filename" is a link, because the code internally uses
> stat().   The function name makes that obvious, but that's not
> commonly known, I guess.  Please see the attached, that I would be
> fine to apply.

Hmm.  I am having second thoughts on this one, as on Windows we rely
on GetFileInformationByHandle() for the emulation of stat() in
win32stat.c, and it looks like this returns some information about the
junction point and not the directory or file this is pointing to, it
seems.  At the end, it looks better to keep things simple here, so
let's drop it.
--
Michael


signature.asc
Description: PGP signature

Re: Column Filtering in Logical Replication

[Tomas Vondra]

On 3/26/22 05:09, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
> Hello, 
> 
> The 'prattrs' column has been added to the pg_publication_rel catalog, 
> but the current commit to catalog.sgml seems to have added it to 
> pg_publication_namespace. 
> The attached patch fixes this.
> 

Thanks, I'll get this pushed.

Sadly, while looking at the catalog docs I realized I forgot to bump the
catversion :-(


regards

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

Re: logical decoding and replication of sequences

[Tomas Vondra]

On 3/26/22 08:28, Amit Kapila wrote:
> On Fri, Mar 25, 2022 at 10:20 PM Tomas Vondra
>  wrote:
>>
>> Hmm, so fixing this might be a bit trickier than I expected.
>>
>> Firstly, currently we only send nspname/relname in the sequence message,
>> not the remote OID or schema. The idea was that for sequences we don't
>> really need schema info, so this seemed OK.
>>
>> But should_apply_changes_for_rel() needs LogicalRepRelMapEntry, and to
>> create/maintain that those records we need to send the schema.
>>
>> Attached is a WIP patch does that.
>>
>> Two places need more work, I think:
>>
>> 1) maybe_send_schema needs ReorderBufferChange, but we don't have that
>> for sequences, we only have TXN. I created a simple wrapper, but maybe
>> we should just tweak maybe_send_schema to use TXN.
>>
>> 2) The transaction handling in is a bit confusing. The non-transactional
>> increments won't have any explicit commit later, so we can't just rely
>> on begin_replication_step/end_replication_step. But I want to try
>> spending a bit more time on this.
>>
> 
> I didn't understand what you want to say in point (2).
> 

My point is that handle_apply_sequence() either needs to use the same
transaction handling as other apply methods, or start (and commit) a
separate transaction for the "transactional" case.

Which means we can't use the begin_replication_step/end_replication_step
and the current code seems a bit complex. And I'm not sure it's quite
correct. So this place needs more work.

>>
>> But there's a more serious issue, I think. So far, we allowed this:
>>
>>   BEGIN;
>>   CREATE SEQUENCE s2;
>>   ALTER PUBLICATION p ADD SEQUENCE s2;
>>   INSERT INTO seq_test SELECT nextval('s2') FROM generate_series(1,100);
>>   COMMIT;
>>
>> and the behavior was that we replicated the changes. But with the patch
>> applied, that no longer happens, because should_apply_changes_for_rel
>> says the change should not be applied.
>>
>> And after thinking about this, I think that's correct - we can't apply
>> changes until ALTER SUBSCRIPTION ... REFRESH PUBLICATION gets executed,
>> and we can't do that until the transaction commits.
>>
>> So I guess that's correct, and the current behavior is a bug.
>>
> 
> Yes, I also think that is a bug.
> 

OK

>> For a while I was thinking that maybe this means we don't need the
>> transactional behavior at all, but I think we do - we have to handle
>> ALTER SEQUENCE cases that are transactional.
>>
> 
> I need some time to think about this.

Understood.

> At all places, it is mentioned
> as creating a sequence for transactional cases which at the very least
> need some tweak.
> 

Which places?



regards

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

Re: pg_dump new feature: exporting functions only. Bad or good idea ?

[Laetitia Avrot]

Hello all,

Le sam. 26 mars 2022 à 01:13, Michael Paquier  a
écrit :

> On Fri, Mar 25, 2022 at 10:09:33PM +0100, Daniel Gustafsson wrote:
> > Agreed.  In this case it seems that adding --exclude-extension would
> make sense
> > to keep conistency.  I took a quick stab at doing so with the attached
> while
> > we're here.
>
> src/test/modules/test_pg_dump would be the best place for the addition
> of a couple of tests with this new switch.  Better to check as well
> what happens when a command collides with --extension and
> --exclude-extension.
>
> printf(_("  -e, --extension=PATTERN  dump the specified
> extension(s) only\n"));
> +   printf(_("  --exclude-extension=PATTERN  do NOT dump the specified
> extension(s)\n"));
> Shouldn't this be listed closer to --exclude-table-data in the --help
> output?
>

I think it's time to sum up what we want to do:

- We'd like to use switches to export objects according to a pattern.
- For each object type we will have an --object=PATTERN flag and a
--exclude-object=PATTERN
- Having a short flag for each of the long flags is not mandatory
- The object types that pg_dump can select so far are:
- table (already written)
- schema (already written)
- extension (half-written, --exclude-extension not written)
- routine (TBD ASAP). Routine flag operates on stored functions, stored
procedures, aggregate functions, and window functions.
- By default, pg_dump does not export system objects but we found out that
we could use --table='pg_catalog.*' to export them. This is a bug and will
be fixed. pg_dump won't have the ability to export any system object
anymore. Should the fix belong to that patch or do I need to create a
separate patch? (Seems to me it should be separated)

If everyone is ok with the points above, I'll write both patches.

Have a nice day,

Lætitia

Re: logical decoding and replication of sequences

[Amit Kapila]

On Fri, Mar 25, 2022 at 10:20 PM Tomas Vondra
 wrote:
>
> Hmm, so fixing this might be a bit trickier than I expected.
>
> Firstly, currently we only send nspname/relname in the sequence message,
> not the remote OID or schema. The idea was that for sequences we don't
> really need schema info, so this seemed OK.
>
> But should_apply_changes_for_rel() needs LogicalRepRelMapEntry, and to
> create/maintain that those records we need to send the schema.
>
> Attached is a WIP patch does that.
>
> Two places need more work, I think:
>
> 1) maybe_send_schema needs ReorderBufferChange, but we don't have that
> for sequences, we only have TXN. I created a simple wrapper, but maybe
> we should just tweak maybe_send_schema to use TXN.
>
> 2) The transaction handling in is a bit confusing. The non-transactional
> increments won't have any explicit commit later, so we can't just rely
> on begin_replication_step/end_replication_step. But I want to try
> spending a bit more time on this.
>

I didn't understand what you want to say in point (2).

>
> But there's a more serious issue, I think. So far, we allowed this:
>
>   BEGIN;
>   CREATE SEQUENCE s2;
>   ALTER PUBLICATION p ADD SEQUENCE s2;
>   INSERT INTO seq_test SELECT nextval('s2') FROM generate_series(1,100);
>   COMMIT;
>
> and the behavior was that we replicated the changes. But with the patch
> applied, that no longer happens, because should_apply_changes_for_rel
> says the change should not be applied.
>
> And after thinking about this, I think that's correct - we can't apply
> changes until ALTER SUBSCRIPTION ... REFRESH PUBLICATION gets executed,
> and we can't do that until the transaction commits.
>
> So I guess that's correct, and the current behavior is a bug.
>

Yes, I also think that is a bug.

> For a while I was thinking that maybe this means we don't need the
> transactional behavior at all, but I think we do - we have to handle
> ALTER SEQUENCE cases that are transactional.
>

I need some time to think about this. At all places, it is mentioned
as creating a sequence for transactional cases which at the very least
need some tweak.

-- 
With Regards,
Amit Kapila.

Re: Remove an unused function GetWalRcvWriteRecPtr

[Julien Rouhaud]

On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
> On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote:
> > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > pg_atomic_read_u64(>writtenUpto); (reading writtenUpto without
> > spinlock) is being used directly in pg_stat_get_wal_receiver
> > walreceiver.c. We either make use of the function instead of
> > pg_atomic_read_u64(>writtenUpto); or remove it. Since there's
> > only one function using walrcv->writtenUpto right now, I prefer to
> > remove the function to save some LOC (13).
> > 
> > Attaching patch. Thoughts?
> 
> This could be used by some external module, no?

Maybe, but WalRcv is exposed so if an external module needs it it could still
maintain its own version of GetWalRcvWriteRecPtr.

Re: make MaxBackends available in _PG_init

[Julien Rouhaud]

Hi,

On Fri, Mar 25, 2022 at 03:23:17PM -0700, Andres Freund wrote:
>
> I don't really understand. The issue that started this thread was bugs in
> extensions due to accessing MaxBackends before it is initialized - which the
> patch prevents.

Well, the patch prevents accessing a 0-valued MaxBackends but doesn't do
anything to solve the original complaint.  It's not like extensions won't need
to access that information during _PG_init anymore.

> The stuff that you're complaining about / designing here
> doesn't seem related to that. I like the idea of the hooks etc, but I fail to
> see why we "ought to revisit the business with GetMaxBackends()"?

Because this GetMaxBackends() doesn't solve the problem nor brings any
infrastructure that could be reused to solve it.

I think that the root issue could be rephrased with:

Can we initialize MaxBackends earlier so that _PG_init() can see it because
maintaining MaxConnections + autovacuum_max_workers + 1 + max_worker_processes
+ max_wal_senders is troublesome.


And indeed, any third party code that previously needed to access what
MaxBackends is supposed to store should already be using that formula, and
the new GetMaxBackends() doesn't do anything about it.  So all extensions will
be as broken as before, except the few that were using MaxBackends without
realizing it's 0.  And if those exist (there's actually one) they're not that
broken, probably because MaxBackend is only used to request additional shmem,
with wanted value small enough so that it's compensated by the extra 100kB
shmem postgres allocates.

Since all those underlying GUCs shouldn't be accessible, we need some more
general infrastructure that would work for those too on top of a way to access
MaxBackends when extensions needs it.

Note that the only use case I'm aware of is for RequestAddinShmemSpace, so a
hook after _PG_init like the example shmem_request_hook would be enough for the
latter, but maybe there are more use cases for which it wouldn't.

Re: Accommodate startup process in a separate ProcState array slot instead of in MaxBackends slots.

[Bharath Rupireddy]

On Sat, Mar 26, 2022 at 1:20 AM Robert Haas  wrote:
>
> On Sat, Feb 12, 2022 at 6:26 AM Bharath Rupireddy
>  wrote:
> > FWIW, here's a patch just adding a comment on how the startup process
> > can get a free procState array slot even when SInvalShmemSize hasn't
> > accounted for it.
>
> I don't think the positioning of this code comment is very good,
> because it's commenting on 0 lines of code. Perhaps that problem could
> be fixed by making it the second paragraph of the immediately
> preceding comment instead of a separate block, but I think the right
> place to comment on this sort of thing is actually in the code that
> sizes the data structure - i.e. SInvalShmemSize. If someone looks at
> that function and says "hey, this uses GetMaxBackends(), that's off by
> one!" they are not ever going to find this comment explaining the
> reasoning.

Thanks. It makes sense to put the comment in SInvalShmemSize.
Attaching v2 patch. Please review it.

Regards,
Bharath Rupireddy.


v2-0001-Add-comment-about-startup-process-getting-procSta.patch
Description: Binary data

Re: Use "WAL segment" instead of "log segment" consistently in user-facing messages

[Bharath Rupireddy]

On Wed, Mar 2, 2022 at 11:41 AM Bharath Rupireddy
 wrote:
>
> Please review the v2 patch set.

Had to rebase. Attaching v3 patch set.

Regards,
Bharath Rupireddy.


v3-0001-Use-WAL-segment-instead-of-log-segment.patch
Description: Binary data


v3-0002-Replace-log-record-with-WAL-record-in-docs.patch
Description: Binary data

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Michael Paquier]

On Thu, Mar 24, 2022 at 05:44:06PM +, Jacob Champion wrote:
> On Wed, 2022-03-23 at 16:54 -0700, Andres Freund wrote:
>> Another option would be to make it a GUC. With a bit of care it could be
>> automatically synced by the existing parallelism infrastructure...
> 
> Like a write-once, PGC_INTERNAL setting? I guess I don't have any
> intuition on how that would compare to the separate-global-and-accessor 
> approach. Is the primary advantage that you don't have to maintain the
> serialization logic, or is there more to it?

Hmm.  That would be a first for a GUC, no?  It is not seem natural
compared to the other information pieces passed down from the leader
to the workers.

+extern SharedPort MyProcShared;

This naming is interesting, and seems to be in line with a couple of
executor structures that share information across workers.  Still
that's a bit inconsistent as Shared is used once at the beginning and
once at the end?  I don't have a better idea on top of my mind.

Anyway, wouldn't it be better to reverse the patch order, introducing
the shared Proc information first and then build the parallel-safe
function on top of it?  
--
Michael


signature.asc
Description: PGP signature

Re: Add LZ4 compression in pg_dump

[Justin Pryzby]

On Sat, Mar 26, 2022 at 02:57:50PM +0900, Michael Paquier wrote:
> I have a question about 0002, actually.  What has led you to the
> conclusion that this code is dead and could be removed?

See 0001 and the manpage.

+   'pg_dump: compression is not supported by tar archive format');

When I submitted a patch to support zstd, I spent awhile trying to make
compression work with tar, but it's a significant effort and better done
separately.

Re: pg_relation_size on partitioned table

[Michael Paquier]

On Fri, Mar 25, 2022 at 08:52:40PM +0800, Japin Li wrote:
> Could we provide a function to get the total size of the partition table
> though the partitioned table name?  Maybe we can extend
> the pg_relation_size() to get the total size of partition tables through
> the partitioned table name.

There are already many replies on this thread, but nobody has
mentioned pg_partition_tree() yet, so here you go.  You could use that
in combination with pg_relation_size() to get the whole size of a tree
depending on your needs.
--
Michael


signature.asc
Description: PGP signature

Re: Corruption during WAL replay

[Michael Paquier]

On Fri, Mar 25, 2022 at 10:34:49AM -0400, Tom Lane wrote:
> Robert Haas  writes:
>> On Fri, Mar 25, 2022 at 10:02 AM Tom Lane  wrote:
>>> Adding another 16 bits won't get you to that, sadly.  Yeah, it *might*
>>> extend the MTTF to more than the project's likely lifespan, but that
>>> doesn't mean we couldn't get unlucky next week.
> 
>> I suspect that the number of bits Andres wants to add is no less than 48.
> 
> I dunno.  Compatibility and speed concerns aside, that seems like an awful
> lot of bits to be expending on every page compared to the value.

Err.  And there are not that many bits that could be recycled for this
purpose in the current page layout, aren't there?
--
Michael


signature.asc
Description: PGP signature