Re: [PATCH] Renumber confusing value for GUC_UNIT_BYTE

[Tom Lane]

Peter Eisentraut  writes:
> I think renumbering this makes sense.  We could just leave the comment 
> as is if we don't come up with a better wording.

+1, I see no need to change the comment.  We just need to establish
the precedent that values within the GUC_UNIT_MEMORY field can be
chosen sequentially.

regards, tom lane

Re: pg_waldump: add test for coverage

[Peter Eisentraut]

On 23.08.22 03:50, Dong Wook Lee wrote:

Hi Hackers,
I wrote a test for coverage.
Unfortunately, it seems to take quite a while to run the test.
I want to improve these execution times, but I don't know exactly what to do.
Therefore, I want to hear feedback from many people.


I don't find these tests to be particularly slow.  How long do they take 
for you to run?


A couple of tips:

- You should give each test a name.  That's why each test function has a 
(usually) last argument that takes a string.


- You could use command_like() to run a command and check that it exits 
successfully and check its standard out.  For example, instead of


# test pg_waldump with -F (main)
IPC::Run::run [ 'pg_waldump', "$wal_dump_path", '-F', 'main' ], '>', 
\$stdout, '2>', \$stderr;

isnt($stdout, '', "");

it is better to write

command_like([ 'pg_waldump', "$wal_dump_path", '-F', 'main' ],
 qr/TODO/, 'test -F (main)');

- It would be useful to test the actual output (that is, fill in the 
TODO above).  I don't know what the best way to do that is -- that is 
part of designing these tests.


Also,

- Your patch introduces a spurious blank line at the end of the test file.

- For portability, options must be before non-option arguments.  So 
instead of


[ 'pg_waldump', "$wal_dump_path", '-F', 'main' ]

it should be

[ 'pg_waldump', '-F', 'main', "$wal_dump_path" ]


I think having some more test coverage for pg_waldump would be good, so 
I encourage you to continue working on this.

Re: more descriptive message for process termination due to max_slot_wal_keep_size

[Kyotaro Horiguchi]

At Mon, 5 Sep 2022 11:56:33 +0200, "Drouvot, Bertrand"  
wrote in 
> Hi,
> 
> On 3/2/22 7:37 AM, Kyotaro Horiguchi wrote:
> > At Tue, 04 Jan 2022 10:29:31 +0900 (JST), Kyotaro
> > Horiguchi wrote in
> >> So what do you say if I propose the following?
> >>
> >> LOG:  terminating process %d to release replication slot \"%s\"
> >> because its restart_lsn %X/%X exceeds the limit %X/%X
> >> HINT: You might need to increase max_slot_wal_keep_size.
> > This version emits the following message.
> >
> > [35785:checkpointer] LOG: terminating process 36368 to release
> > replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
> > limit 0/2100
> > [35785:checkpointer] HINT: You might need to increase
> > max_slot_wal_keep_size.
> 
> As the hint is to increase max_slot_wal_keep_size, what about
> reporting the difference in size (rather than the limit lsn)?
> Something along those lines?
> 
> [35785:checkpointer] LOG: terminating process 36368 to release
> replication slot "s1" because its restart_lsn 0/1F000148 exceeds the
> limit by .

Thanks! That might be more sensible exactly for the reason you
mentioned.  One issue doing that is size_pretty is dbsize.c local
function. Since the size is less than kB in many cases, we cannot use
fixed unit for that.

0001 and 0002 are the same with v5.

0003 exposes byte_size_pretty() to other modules.
0004 does the change by using byte_size_pretty()

After 0004 applied, they look like this.

> LOG:  terminating process 108413 to release replication slot "rep3" because 
> its restart_lsn 0/7000D8 exceeds the limit by 1024 kB
> HINT:  You might need to increase max_slot_wal_keep_size.

The reason for "1024 kB" instead of "1 MB" is the precise value is a
bit less than 1024 * 1024.


regards.

- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From d6432aa13d3f4446a5cee4c4c33dcf5841314546 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 24 Dec 2021 12:52:07 +0900
Subject: [PATCH v6 1/4] Make a message on process termination more dscriptive

The message at process termination due to slot limit doesn't provide
the reason. In the major scenario the message is followed by another
message about slot invalidatation, which shows the detail for the
termination.  However the second message is missing if the slot is
temporary one.

Augment the first message with the reason same as the second message.

Backpatch through to 13 where the message was introduced.

Reported-by: Alex Enachioaie 
Author: Kyotaro Horiguchi 
Reviewed-by: Ashutosh Bapat 
Reviewed-by: Bharath Rupireddy 
Discussion: https://www.postgresql.org/message-id/17327-89d0efa8b9ae6271%40postgresql.org
Backpatch-through: 13
---
 src/backend/replication/slot.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 8fec1cb4a5..8326c019cf 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1293,8 +1293,9 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr oldestLSN,
 			if (last_signaled_pid != active_pid)
 			{
 ereport(LOG,
-		(errmsg("terminating process %d to release replication slot \"%s\"",
-active_pid, NameStr(slotname;
+		(errmsg("terminating process %d to release replication slot \"%s\" because its restart_lsn %X/%X exceeds max_slot_wal_keep_size",
+active_pid, NameStr(slotname),
+LSN_FORMAT_ARGS(restart_lsn;
 
 (void) kill(active_pid, SIGTERM);
 last_signaled_pid = active_pid;
-- 
2.31.1

>From da3df65d4a24d6235b401aa4f944a0f4d3652207 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 24 Dec 2021 12:58:25 +0900
Subject: [PATCH v6 2/4] Add detailed information to slot-invalidation messages

The messages says just "exceeds max_slot_wal_keep_size" but doesn't
tell the concrete LSN of the limit. That information helps operators'
understanding on the issue.

Author: Kyotaro Horiguchi 
Reviewed-by: Ashutosh Bapat 
Reviewed-by: Masahiko Sawada 
Discussion: https://www.postgresql.org/message-id/17327-89d0efa8b9ae6271%40postgresql.org
---
 src/backend/replication/slot.c| 12 
 src/test/recovery/t/019_replslot_limit.pl |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 8326c019cf..a30d4e93cd 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1293,9 +1293,11 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr oldestLSN,
 			if (last_signaled_pid != active_pid)
 			{
 ereport(LOG,
-		(errmsg("terminating process %d to release replication slot \"%s\" because its restart_lsn %X/%X exceeds max_slot_wal_keep_size",
+		(errmsg("terminating process %d to release replication slot \"%s\" because its restart_lsn %X/%X exceeds the limit %X/%X",
 active_pid, NameStr(slotname),
-LSN_FORMAT_ARGS(restart_lsn;
+

Re: Patch to address creation of PgStat* contexts with null parent context

[Kyotaro Horiguchi]

(It seems to me I overlooked some mails.. sorry.)

At Mon, 5 Sep 2022 15:47:37 -0700, Andres Freund  wrote in 
> On 2022-09-05 17:32:20 +0900, Kyotaro Horiguchi wrote:
> > The rationale of creating them at pgstat_attach_shmem is that anyway once
> > pgstat_attach_shmem is called, the process fainally creates the contexts at
> > the end of the process, and (I think) it's simpler that we don't do if()
> > check at every pgstat_get_entry_ref() call.
> 
> But that's not true, as pointed out here:
> https://postgr.es/m/20220808192020.nc556tlgcp66fdgw%40awork3.anarazel.de
> 
> Nor does it make sense to reserve memory for the entire lifetime of a process
> just because we might need it for a split second at the end.

Yeah, that's the most convincing argument aginst it.

At Mon, 5 Sep 2022 14:46:55 +0200, "Drouvot, Bertrand"  
wrote in 
> Looks like that both approaches have their pros and cons. I'm tempted
> to vote +1 on "just changing" the parent context to TopMemoryContext
> and not changing the allocations locations.

Yeah. It is safe more than anything and we don't have a problem there.

So, I'm fine with just replacing the parent context at the three places.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH] Renumber confusing value for GUC_UNIT_BYTE

[Peter Eisentraut]

On 20.07.22 16:52, Justin Pryzby wrote:

+/* GUC_UNIT_* are not flags - they're tested for equality */


Well, there is GUC_UNIT_MEMORY, etc. so there is an additional 
constraint beyond just "pick any number".  I'm not sure that "flag" and 
"tested for equality" are really antonyms anyway.


I think renumbering this makes sense.  We could just leave the comment 
as is if we don't come up with a better wording.



  #define GUC_UNIT_KB   0x1000  /* value is in 
kilobytes */
  #define GUC_UNIT_BLOCKS   0x2000  /* value is in blocks */
  #define GUC_UNIT_XBLOCKS  0x3000  /* value is in xlog blocks */
  #define GUC_UNIT_MB   0x4000  /* value is in 
megabytes */
-#define GUC_UNIT_BYTE  0x8000  /* value is in bytes */
+#define GUC_UNIT_BYTE  0x5000  /* value is in bytes */
  #define GUC_UNIT_MEMORY   0xF000  /* mask for 
size-related units */
  
  #define GUC_UNIT_MS			   0x1	/* value is in milliseconds */

Re: Modernizing our GUC infrastructure

[Tom Lane]

Pavel Stehule  writes:
> út 6. 9. 2022 v 6:32 odesílatel Pavel Stehule 
> napsal:
>> 1. Session variables can be persistent - so the usage of session variables
>> can be checked by static analyze like plpgsql_check

> more precious - metadata of session variables are persistent

Right ... so the question is, is that a feature or a bug?

I think there's a good analogy here to temporary tables.  The SQL
spec says that temp-table schemas are persistent and database-wide,
but what we actually have is that they are session-local.  People
occasionally propose that we implement the SQL semantics for that,
but in the last twenty-plus years no one has bothered to write a
committable patch to support it ... much less remove the existing
behavior in favor of that, which I'm pretty sure no one would think
is a good idea.

So, is it actually a good idea to have persistent metadata for
session variables?  I'd say that the issue is at best debatable,
and at worst proven wrong by a couple of decades of experience.
In what way are session variables less mutable than temp tables?

Still, this discussion would be better placed on the other thread.

regards, tom lane

Re: Modernizing our GUC infrastructure

[Tom Lane]

Pavel Stehule  writes:
> The bad performance is not the main reason for implementing session
> variables (and in almost all cases the performance of GUC is not a problem,
> because it is not a bottleneck, and in some terrible cases, I can save the
> GUC to a variable). There are other differences:

Well, yeah, the schema-variables patch offers a bunch of other features.
What I'm not sure about is whether there's actually much field demand
for those.  I think if we fix guc.c's performance issues and add some
simple features on top of that, like the ability to declare bool, int,
float data types not just string for a user-defined GUC, we'd have
exactly what a lot of people want --- not least because it'd be
upwards-compatible with what they are already doing.

However, that's probably a debate to have on the other thread not here.
This patch doesn't foreclose pushing forward with the schema-variables
patch, if people want that.

regards, tom lane

Re: Handle infinite recursion in logical replication setup

[Peter Smith]

Thanks for addressing my previous review comments. I have no more
comments for v46*.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Modernizing our GUC infrastructure

[Julien Rouhaud]

Hi,

On Tue, Sep 06, 2022 at 06:32:21AM +0200, Pavel Stehule wrote:
> Hi
>
> út 6. 9. 2022 v 0:28 odesílatel Tom Lane  napsal:
>
> > Attached is a patch series that attempts to modernize our GUC
> > infrastructure, in particular removing the performance bottlenecks
> > it has when there are lots of GUC variables.  I wrote this because
> > I am starting to question the schema-variables patch [1] --- that's
> > getting to be quite a large patch and I grow less and less sure
> > that it's solving a problem our users want solved.  I think what
> > people actually want is better support of the existing mechanism
> > for ad-hoc session variables, namely abusing custom GUCs for that
> > purpose.  One of the big reasons we have been resistant to formally
> > supporting that is fear of the poor performance guc.c would have
> > with lots of variables.  But we already have quite a lot of them:
> >
> >
> The bad performance is not the main reason for implementing session
> variables (and in almost all cases the performance of GUC is not a problem,
> because it is not a bottleneck, and in some terrible cases, I can save the
> GUC to a variable). There are other differences:
>
> 1. Session variables metadata can be persistent - so the usage of session
> variables can be checked by static analyze like plpgsql_check
>
> 2. Session variables supports not atomic data types - so the work with row
> types or arrays is much more comfortable and faster, because there is no
> conversion string <-> binary
>
> 3. Session variables allows to set access rights
>
> 4. Session variables are nullable and allowed to specify default values.
>
> I don't think so users have ten thousand GUC and the huge count of GUC is
> the main performance problem. The source of the performance problem is
> storing the value only as string.

I think we can also mention those differences with the proposed schema
variables:

- schema variables have normal SQL integration, having to use current_setting()
  isn't ideal (on top of only supporting text) and doesn't really play nice
  with pg_stat_statements for instance

- schema variables implement stability in a single SQL statement (not in
  plpgsql), while current_setting always report the latest set value.  This one
  may or may not be wanted, and maybe the discrepancy with procedural languages
  would be too problematic, but it's still something proposed

Re: Modernizing our GUC infrastructure

[Pavel Stehule]

út 6. 9. 2022 v 6:32 odesílatel Pavel Stehule 
napsal:

> Hi
>
> út 6. 9. 2022 v 0:28 odesílatel Tom Lane  napsal:
>
>> Attached is a patch series that attempts to modernize our GUC
>> infrastructure, in particular removing the performance bottlenecks
>> it has when there are lots of GUC variables.  I wrote this because
>> I am starting to question the schema-variables patch [1] --- that's
>> getting to be quite a large patch and I grow less and less sure
>> that it's solving a problem our users want solved.  I think what
>> people actually want is better support of the existing mechanism
>> for ad-hoc session variables, namely abusing custom GUCs for that
>> purpose.  One of the big reasons we have been resistant to formally
>> supporting that is fear of the poor performance guc.c would have
>> with lots of variables.  But we already have quite a lot of them:
>>
>>
> The bad performance is not the main reason for implementing session
> variables (and in almost all cases the performance of GUC is not a problem,
> because it is not a bottleneck, and in some terrible cases, I can save the
> GUC to a variable). There are other differences:
>
> 1. Session variables can be persistent - so the usage of session variables
> can be checked by static analyze like plpgsql_check
>

more precious - metadata of session variables are persistent


>
> 2. Session variables supports not atomic data types - so the work with row
> types or arrays is much more comfortable and faster, because there is no
> conversion string <-> binary
>
> 3. Session variables allows to set access rights
>
> 4. Session variables are nullable and allowed to specify default values.
>
> I don't think so users have ten thousand GUC and the huge count of GUC is
> the main performance problem. The source of the performance problem is
> storing the value only as string.
>
> Regards
>
> Pavel
>
>
>

Re: Modernizing our GUC infrastructure

[Pavel Stehule]

Hi

út 6. 9. 2022 v 0:28 odesílatel Tom Lane  napsal:

> Attached is a patch series that attempts to modernize our GUC
> infrastructure, in particular removing the performance bottlenecks
> it has when there are lots of GUC variables.  I wrote this because
> I am starting to question the schema-variables patch [1] --- that's
> getting to be quite a large patch and I grow less and less sure
> that it's solving a problem our users want solved.  I think what
> people actually want is better support of the existing mechanism
> for ad-hoc session variables, namely abusing custom GUCs for that
> purpose.  One of the big reasons we have been resistant to formally
> supporting that is fear of the poor performance guc.c would have
> with lots of variables.  But we already have quite a lot of them:
>
>
The bad performance is not the main reason for implementing session
variables (and in almost all cases the performance of GUC is not a problem,
because it is not a bottleneck, and in some terrible cases, I can save the
GUC to a variable). There are other differences:

1. Session variables can be persistent - so the usage of session variables
can be checked by static analyze like plpgsql_check

2. Session variables supports not atomic data types - so the work with row
types or arrays is much more comfortable and faster, because there is no
conversion string <-> binary

3. Session variables allows to set access rights

4. Session variables are nullable and allowed to specify default values.

I don't think so users have ten thousand GUC and the huge count of GUC is
the main performance problem. The source of the performance problem is
storing the value only as string.

Regards

Pavel

Re: Compilation issue on Solaris.

[John Naylor]

On Sun, Jul 10, 2022 at 9:27 PM Tom Lane  wrote:
>
> Thomas Munro  writes:
> > Something bothers me about adding yet more clutter to every compile
> > line for the rest of time to solve a problem that exists only for
> > unpatched systems, and also that it's not even really a Solaris thing,
> > it's a C11 thing.
>
> I tend to agree with this standpoint: if it's only a warning, and
> it only appears in a small range of not-up-to-date Solaris builds,
> then a reasonable approach is "update your system if you don't want
> to see the warning".
>
> A positive argument for doing nothing is that there's room to worry
> whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we
> *don't* want.

This is still listed in the CF as needing review, so I went and marked
it rejected.

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

RE: Handle infinite recursion in logical replication setup

[wangw.f...@fujitsu.com]

On Tues, 6 Sept 2022 at 11:14, vignesh C  wrote:
> Thanks for the comments, the attached patch has the changes for the same.

Thanks for updating the patch.

Here is one comment for 0001 patch.
1. The query in function check_publications_origin.
+   appendStringInfoString(,
+  "SELECT DISTINCT P.pubname 
AS pubname\n"
+  "FROM pg_publication P,\n"
+  " LATERAL 
pg_get_publication_tables(P.pubname) GPT\n"
+  " LEFT JOIN 
pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
+  " pg_class C JOIN 
pg_namespace N ON (N.oid = C.relnamespace)\n"
+  "WHERE C.oid = GPT.relid AND 
PS.srrelid IS NOT NULL AND P.pubname IN (");

Since I found that we only use "PS.srrelid" in the WHERE statement by
specifying "PS.srrelid IS NOT NULL", could we just use "[INNER] JOIN" to join
the table pg_subscription_rel?

Regards,
Wang wei

RE: Handle infinite recursion in logical replication setup

[shiy.f...@fujitsu.com]

On Tue, Sep 6, 2022 11:14 AM vignesh C  wrote:
> 
> Thanks for the comments, the attached patch has the changes for the same.
> 

Thanks for updating the patch. Here are some comments.

1.
+   if (subrel_count)
+   {
+   /*
+* In case of ALTER SUBSCRIPTION ... REFRESH, subrel_local_oids
+* contains the list of relation oids that are already present 
on the
+* subscriber. This check should be skipped for these tables.
+*/
+   appendStringInfo(, "AND C.oid NOT IN (\n"
+"SELECT C.oid \n"
+"FROM pg_class C\n"
+" JOIN pg_namespace N ON 
N.oid = C.relnamespace\n"
+"WHERE ");
+   get_skip_tables_str(subrel_local_oids, subrel_count, );
+   appendStringInfo(, ")");
+   }

I think we can skip the tables without using a subquery. See the SQL below.
Would it be better?

SELECT DISTINCT P.pubname AS pubname
FROM pg_publication P,
 LATERAL pg_get_publication_tables(P.pubname) GPT
 LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
 pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND P.pubname IN ('p1', 'p3')
AND NOT (N.nspname = 'public' AND C.relname = 'tbl')
AND NOT (N.nspname = 'public' AND C.relname = 't1');


2.
+   When using a subscription parameter combination of
+   copy_data=true and origin=NONE, the

Could we use "copy_data = true" and "origin = NONE" (add two spaces around the
equals sign)? I think it looks clearer that way. And it is consistent with other
places in this file.

Regards,
Shi yu

Re: FOR EACH ROW triggers, on partitioend tables, with indexes?

[David Rowley]

On Thu, 1 Sept 2022 at 20:57, Alvaro Herrera  wrote:
> So apparently the way to get a trigger associated with a relation
> (tgconstrrelid) is via CREATE CONSTRAINT TRIGGER, but there doesn't
> appear to be a way to have it associated with a specific *index* on that
> relation (tgconstrindid).  So you're right that it appears to be dead
> code.
>
> If the regression tests don't break by removing it, I agree with doing
> that.

Thanks for having a look here. Yeah, it was a while ago.

I've pushed a patch to remove the dead code from master. I don't quite
see the sense in removing it in the back branches.

David

Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Tue, 6 Sept 2022 at 14:43, Tom Lane  wrote:
> I think MemoryContextContains' charter is to return
>
> GetMemoryChunkContext(pointer) == context
>
> *except* that instead of asserting what GetMemoryChunkContext asserts,
> it should treat those cases as reasons to return false.  So if you
> can still do GetMemoryChunkContext then you can still do
> MemoryContextContains.  The point of having the separate function
> is to be as forgiving as we can of bogus pointers.

Ok.  I've readded the Asserts that c6e0fe1f2 mistakenly removed from
GetMemoryChunkContext() and changed MemoryContextContains() to do
those same pre-checks before calling GetMemoryChunkContext().

I've also boosted the Assert in mcxt.c to
Assert(MemoryContextContains(context, ret)) in each place we call the
context's callback function to obtain a newly allocated pointer.  I
think this should cover the testing.

I felt the need to keep the adjustments I made to the header comment
in MemoryContextContains() to ward off anyone who thinks it's ok to
pass this any random pointer and have it do something sane. It's much
more prone to misbehaving/segfaulting now given the extra dereferences
that c6e0fe1f2 added to obtain a pointer to the owning context.

David
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 64bcc7ef32..3d80abbfae 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -482,6 +482,15 @@ MemoryContextAllowInCriticalSection(MemoryContext context, 
bool allow)
 MemoryContext
 GetMemoryChunkContext(void *pointer)
 {
+   /*
+* Try to detect bogus pointers handed to us, poorly though we can.
+* Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+* allocated chunk.
+*/
+   Assert(pointer != NULL);
+   Assert(pointer == (void *) MAXALIGN(pointer));
+   /* adding further Asserts here? See pre-checks in MemoryContextContains 
*/
+
return MCXT_METHOD(pointer, get_chunk_context) (pointer);
 }
 
@@ -809,11 +818,10 @@ MemoryContextCheck(MemoryContext context)
  * Detect whether an allocated chunk of memory belongs to a given
  * context or not.
  *
- * Caution: this test is reliable as long as 'pointer' does point to
- * a chunk of memory allocated from *some* context.  If 'pointer' points
- * at memory obtained in some other way, there is a small chance of a
- * false-positive result, since the bits right before it might look like
- * a valid chunk header by chance.
+ * Caution: 'pointer' must point to a pointer which was allocated by a
+ * MemoryContext.  It's not safe or valid to use this function on arbitrary
+ * pointers as obtaining the MemoryContext which 'pointer' belongs to requires
+ * possibly several pointer dereferences.
  */
 bool
 MemoryContextContains(MemoryContext context, void *pointer)
@@ -821,9 +829,8 @@ MemoryContextContains(MemoryContext context, void *pointer)
MemoryContext ptr_context;
 
/*
-* NB: Can't use GetMemoryChunkContext() here - that performs assertions
-* that aren't acceptable here since we might be passed memory not
-* allocated by any memory context.
+* NB: We must perform run-time checks here which 
GetMemoryChunkContext()
+* does as assertions before calling GetMemoryChunkContext().
 *
 * Try to detect bogus pointers handed to us, poorly though we can.
 * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
@@ -835,7 +842,7 @@ MemoryContextContains(MemoryContext context, void *pointer)
/*
 * OK, it's probably safe to look at the context.
 */
-   ptr_context = *(MemoryContext *) (((char *) pointer) - sizeof(void *));
+   ptr_context = GetMemoryChunkContext(pointer);
 
return ptr_context == context;
 }
@@ -953,6 +960,8 @@ MemoryContextAlloc(MemoryContext context, Size size)
 
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
+   Assert(MemoryContextContains(context, ret));
+
return ret;
 }
 
@@ -991,6 +1000,8 @@ MemoryContextAllocZero(MemoryContext context, Size size)
 
MemSetAligned(ret, 0, size);
 
+   Assert(MemoryContextContains(context, ret));
+
return ret;
 }
 
@@ -1029,6 +1040,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size 
size)
 
MemSetLoop(ret, 0, size);
 
+   Assert(MemoryContextContains(context, ret));
+
return ret;
 }
 
@@ -1070,6 +1083,8 @@ MemoryContextAllocExtended(MemoryContext context, Size 
size, int flags)
if ((flags & MCXT_ALLOC_ZERO) != 0)
MemSetAligned(ret, 0, size);
 
+   Assert(MemoryContextContains(context, ret));
+
return ret;
 }
 
@@ -1153,6 +1168,8 @@ palloc(Size size)
 
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
+   Assert(MemoryContextContains(context, ret));
+
return ret;
 }
 
@@ -1186,6 +1203,8 @@ palloc0(Size size)
 
MemSetAligned(ret, 0, size);
 
+ 

Re: Handle infinite recursion in logical replication setup

[vignesh C]

On Mon, 5 Sept 2022 at 15:10, Amit Kapila  wrote:
>
> On Mon, Sep 5, 2022 at 9:47 AM Peter Smith  wrote:
> >
> > Here are my review comments for v45-0001:
> >
> > ==
> >
> > 1. doc/src/sgml/logical-replication.sgml
> >
> >   
> >To find which tables might potentially include non-local origins (due to
> >other subscriptions created on the publisher) try this SQL query:
> > 
> > SELECT DISTINCT N.nspname AS schemaname, C.relname AS tablename
> > FROM pg_publication P,
> >  LATERAL pg_get_publication_tables(P.pubname) GPT
> >  LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
> >  pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
> > WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND
> >   P.pubname IN (pub-names);
> > 
> >
> > 1a.
> > To use "" with the <> then simply put meta characters in the 
> > SGML.
> > e.g.
> > pub-names
> >
> > ~
> >
> > 1b.
> > The patch forgot to add the SQL "#" instruction as suggested by my
> > previous comment (see [1] #3b)
> >
> > ~~~
> >
> > 2.
> >
> >  
> >   Preventing Data Inconsistencies for copy_data, origin=NONE
> >
> > The title is OK, but I think this should all be a  sub-section
> > of "31.2 Subscription"
> >
> > ==
> >
>
> It is recommended to create the subscription
> +   using enabled=false, so that if the origin WARNING 
> occurs
> +   no copy has happened yet. Otherwise some corrective steps might be needed 
> to
> +   remove any unwanted data that got copied.
>
> I am not completely sure of this part of the docs as this can add
> additional steps for users while working on subscriptions even when
> the same is not required. I suggest for now we can remove this part.
> Later based on some feedback on this feature, we can extend the docs
> if required.

Modified

> Also, instead of having it as a separate section, let's keep this as
> part of create_subscription.sgml

Modified

> *
> + errhint("Verify that initial data copied from the publisher tables
> did not come from other origins. Some corrective action may be
> necessary."));
>
> The second sentence in this message doesn't seem to be required.

Modified

Thanks for the comments, the v46 patch at [1] has the changes for the same.
[1] - 
https://www.postgresql.org/message-id/CALDaNm3TTGdCCkeDsN8hqtF_2z-8%2B%3D3tc9Gh5xOKAQ_BRMCkMA%40mail.gmail.com

Regards,
Vignesh

Re: Handle infinite recursion in logical replication setup

[vignesh C]

On Mon, 5 Sept 2022 at 09:47, Peter Smith  wrote:
>
> Here are my review comments for v45-0001:
>
> ==
>
> 1. doc/src/sgml/logical-replication.sgml
>
>   
>To find which tables might potentially include non-local origins (due to
>other subscriptions created on the publisher) try this SQL query:
> 
> SELECT DISTINCT N.nspname AS schemaname, C.relname AS tablename
> FROM pg_publication P,
>  LATERAL pg_get_publication_tables(P.pubname) GPT
>  LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
>  pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
> WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND
>   P.pubname IN (pub-names);
> 
>
> 1a.
> To use "" with the <> then simply put meta characters in the SGML.
> e.g.
> pub-names

Modified

> ~
>
> 1b.
> The patch forgot to add the SQL "#" instruction as suggested by my
> previous comment (see [1] #3b)

Modified

> ~~~
>
> 2.
>
>  
>   Preventing Data Inconsistencies for copy_data, origin=NONE
>
> The title is OK, but I think this should all be a  sub-section
> of "31.2 Subscription"

I have moved it to create subscription notes based on a recent comment
from Amit.

> ==
>
> 3. src/backend/commands/subscriptioncmds.c - check_publications_origin
>
> + initStringInfo();
> + appendStringInfoString(,
> +"SELECT DISTINCT P.pubname AS pubname\n"
> +"FROM pg_publication P,\n"
> +" LATERAL pg_get_publication_tables(P.pubname) GPT\n"
> +" LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
> +" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
> +"WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND P.pubname IN (");
> + get_publications_str(publications, , true);
> + appendStringInfoChar(, ')');
> + get_skip_tables_str(subrel_local_oids, subrel_count, );
>
> (see from get_skip_tables_str)
>
> + appendStringInfoString(dest, " AND C.oid NOT IN (SELECT C.oid FROM
> pg_class C JOIN pg_namespace N on N.oid = C.relnamespace where ");
>
>
> IMO the way you are using get_skip_tables_str should be modified. I
> will show by way of example below.
> - "where" -> "WHERE"
> - put the SELECT at the caller instead of inside the function
> - handle the ")" at the caller
>
> All this will also make the body of the 'get_skip_tables_str' function
> much simpler (see the next review comments)
>
> SUGGESTION
> if (subrel_count > 0)
> {
> /* TODO - put some explanatory comment here about skipping the tables */
> appendStringInfo(,
> " AND C.oid NOT IN (\n"
> "SELECT C.oid FROM pg_class C\n"
> "JOIN pg_namespace N on N.oid = C.relnamespace WHERE ");
> get_skip_tables_str(subrel_local_oids, subrel_count, );
> appendStringInf(, “)”);
> }

Modified

> ~~~
>
> 4. src/backend/commands/subscriptioncmds.c - get_skip_tables_str
>
> +/*
> + * Add the table names that should be skipped.
> + */
>
> This comment does not have enough detail to know really what the
> function is for. Perhaps you only need to say that this is a helper
> function for 'check_publications_origin' and then where it is called
> you can give a comment (e.g. see my review comment #3)

Modified

> ~~
>
> 5. get_skip_tables_str (body)
>
> 5a. Variable 'count' is not a very good name; IMO just say 'i' for
> index, and it can be declared C99 style.

Modified

> ~
>
> 5b. Variable 'first' is not necessary - it's same as (i == 0)

Modified

> ~
>
> 5c. The whole "SELECT" part and the ")" parts are more simply done at
> the caller (see the review comment #3)

Modified

> ~
>
> 5d.
>
> + appendStringInfo(dest, "(C.relname = '%s' and N.nspname = '%s')",
> + tablename, schemaname);
>
> It makes no difference but I thought it would feel more natural if the
> SQL would compare the schema name *before* the table name.

Modified

> ~
>
> 5e.
> "and" -> "AND"

Modified

> ~
>
> Doing all 5a,b,c,d, and e means overall having a much simpler function body:
>
> SUGGESTION
> + for (int i = 0; i < subrel_count; i++)
> + {
> + Oid relid = subrel_local_oids[i];
> + char*schemaname = get_namespace_name(get_rel_namespace(relid));
> + char*tablename = get_rel_name(relid);
> +
> + if (i > 0)
> + appendStringInfoString(dest, " OR ");
> +
> + appendStringInfo(dest, "(N.nspname = '%s' AND C.relname = '%s')",
> + schemaname, tablename);
> + }

Modified

Thanks for the comments, the attached patch has the changes for the same.

Regards,
Vignesh


v46-0001-Check-and-log-a-warning-if-the-publisher-has-sub.patch
Description: Binary data


v46-0002-Document-the-steps-for-replication-between-prima.patch
Description: Binary data

Re: pg_publication_tables show dropped columns

[Tom Lane]

Jaime Casanova  writes:
> Just trying the new column/row filter on v15, I found this issue that
> could be replicated very easily.

Bleah.  Post-beta4 catversion bump, here we come.

> This could be solved by adding a "NOT attisdropped", simple patch
> attached.

That view seems quite inefficient as written --- I wonder if we
can't do better by nuking the join-to-unnest business and putting
the restriction in a WHERE clause on the pg_attribute scan.
The query plan that you get for it right now is certainly awful.

regards, tom lane

Re: Modernizing our GUC infrastructure

[Junwang Zhao]

ah, yes, that makes sense ;)

On Tue, Sep 6, 2022 at 10:48 AM Tom Lane  wrote:
>
> Junwang Zhao  writes:
> >   /*
> > - * Create table with 20% slack
> > + * Create hash table with 20% slack
> >   */
> >   size_vars = num_vars + num_vars / 4;
>
> > Should we change 20% to 25%, I thought that might be
> > a typo.
>
> No ... 20% of the allocated space is spare.
>
> regards, tom lane



-- 
Regards
Junwang Zhao

pg_publication_tables show dropped columns

[Jaime Casanova]

Hi everyone,

Just trying the new column/row filter on v15, I found this issue that
could be replicated very easily.

"""
postgres=# create table t1(i serial primary key);
CREATE TABLE
postgres=# alter table t1 drop i;
ALTER TABLE
postgres=# alter table t1 add id serial primary key;
ALTER TABLE
postgres=# create publication pub_t1 for table t1;
CREATE PUBLICATION

postgres=# select * from pg_publication_tables where pubname = 'pub_t1' \gx
-[ RECORD 1 ]-
pubname| pub_t1
schemaname | public
tablename  | t1
attnames   | {pg.dropped.1,id}
rowfilter  |
"""

This could be solved by adding a "NOT attisdropped", simple patch
attached.

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index fedaed533b..431864648c 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -369,7 +369,7 @@ CREATE VIEW pg_publication_tables AS
 P.pubname AS pubname,
 N.nspname AS schemaname,
 C.relname AS tablename,
-( SELECT array_agg(a.attname ORDER BY a.attnum)
+( SELECT array_agg(a.attname ORDER BY a.attnum) FILTER (WHERE NOT a.attisdropped)
   FROM unnest(CASE WHEN GPT.attrs IS NOT NULL THEN GPT.attrs
   ELSE (SELECT array_agg(g) FROM generate_series(1, C.relnatts) g)
   END) k

Re: Modernizing our GUC infrastructure

[Tom Lane]

Junwang Zhao  writes:
>   /*
> - * Create table with 20% slack
> + * Create hash table with 20% slack
>   */
>   size_vars = num_vars + num_vars / 4;

> Should we change 20% to 25%, I thought that might be
> a typo.

No ... 20% of the allocated space is spare.

regards, tom lane

Re: Modernizing our GUC infrastructure

[Junwang Zhao]

Hi Tom,

@@ -5836,74 +5865,106 @@ build_guc_variables(void)
  }

  /*
- * Create table with 20% slack
+ * Create hash table with 20% slack
  */
  size_vars = num_vars + num_vars / 4;

Should we change 20% to 25%, I thought that might be
a typo.

On Tue, Sep 6, 2022 at 6:28 AM Tom Lane  wrote:
>
> Attached is a patch series that attempts to modernize our GUC
> infrastructure, in particular removing the performance bottlenecks
> it has when there are lots of GUC variables.  I wrote this because
> I am starting to question the schema-variables patch [1] --- that's
> getting to be quite a large patch and I grow less and less sure
> that it's solving a problem our users want solved.  I think what
> people actually want is better support of the existing mechanism
> for ad-hoc session variables, namely abusing custom GUCs for that
> purpose.  One of the big reasons we have been resistant to formally
> supporting that is fear of the poor performance guc.c would have
> with lots of variables.  But we already have quite a lot of them:
>
> regression=# select count(*) from pg_settings;
>  count
> ---
>353
> (1 row)
>
> and more are getting added all the time.  I think this patch series
> could likely be justified just in terms of positive effect on core
> performance, never mind user-added GUCs.
>
> 0001 and 0002 below are concerned with converting guc.c to store its
> data in a dedicated memory context (GUCMemoryContext) instead of using
> raw malloc().  This is not directly a performance improvement, and
> I'm prepared to drop the idea if there's a lot of pushback, but I
> think it would be a good thing to do.  The only hard reason for using
> malloc() there was the lack of ability to avoid throwing elog(ERROR)
> on out-of-memory in palloc().  But mcxt.c grew that ability years ago.
> Switching to a dedicated context would greatly improve visibility and
> accountability of GUC-related allocations.  Also, the 0003 patch will
> switch guc.c to relying on a palloc-based hashtable, and it seems a
> bit silly to have part of the data structure in palloc and part in
> malloc.  However 0002 is a bit invasive, in that it forces code
> changes in GUC check callbacks, if they want to reallocate the new
> value or create an "extra" data structure.  My feeling is that not
> enough external modules use those facilities for this to pose a big
> problem.  However, the ones that are subject to it will have a
> non-fun time tracking down why their code is crashing.  (The recent
> context-header changes mean that you get a very obscure failure when
> trying to pfree() a malloc'd chunk -- for me, that typically ends
> in an assertion failure in generation.c.  Can we make that less
> confusing?)
>
> 0003 replaces guc.c's bsearch-a-sorted-array lookup infrastructure
> with a dynahash hash table.  (I also looked at simplehash, but it
> has no support for not elog'ing on OOM, and it increases the size
> of guc.o by 10KB or so.)  This fixes the worse-than-O(N^2) time
> needed to create N new GUCs, as in
>
> do $$
> begin
>   for i in 1..1 loop
> perform set_config('foo.bar' || i::text, i::text, false);
>   end loop;
> end $$;
>
> On my machine, this takes about 4700 ms in HEAD, versus 23 ms
> with this patch.  However, the places that were linearly scanning
> the array now need to use hash_seq_search, so some other things
> like transaction shutdown (AtEOXact_GUC) get slower.
>
> To address that, 0004 adds some auxiliary lists that link together
> just the variables that are interesting for specific purposes.
> This is helpful even without considering the possibility of a
> lot of user-added GUCs: in a typical session, for example, not
> many of those 353 GUCs have non-default values, and even fewer
> get modified in any one transaction (typically, anyway).
>
> As an example of the speedup from 0004, these DO loops:
>
> create or replace function func_with_set(int) returns int
> strict immutable language plpgsql as
> $$ begin return $1; end $$
> set enable_seqscan = false;
>
> do $$
> begin
>   for i in 1..10 loop
> perform func_with_set(i);
>   end loop;
> end $$;
>
> do $$
> begin
>   for i in 1..10 loop
> begin
>   perform func_with_set(i);
> exception when others then raise;
> end;
>   end loop;
> end $$;
>
> take about 260 and 320 ms respectively for me, in HEAD with
> just the stock set of variables.  But after creating 1
> GUCs with the previous DO loop, they're up to about 3200 ms.
> 0004 brings that back down to being indistinguishable from the
> speed with few GUCs.
>
> So I think this is good cleanup in its own right, plus it
> removes one major objection to considering user-defined GUCs
> as a supported feature.
>
> regards, tom lane
>
> [1] 
> https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com
>


-- 
Regards
Junwang Zhao

Re: Reducing the chunk header sizes on all memory context types

[Tom Lane]

David Rowley  writes:
> I think the fix is harder than I thought, or perhaps impossible to do
> given how we now determine the owning MemoryContext of a pointer.

> There's a comment in MemoryContextContains which says:

> * NB: Can't use GetMemoryChunkContext() here - that performs assertions
> * that aren't acceptable here since we might be passed memory not
> * allocated by any memory context.

I think MemoryContextContains' charter is to return

GetMemoryChunkContext(pointer) == context

*except* that instead of asserting what GetMemoryChunkContext asserts,
it should treat those cases as reasons to return false.  So if you
can still do GetMemoryChunkContext then you can still do
MemoryContextContains.  The point of having the separate function
is to be as forgiving as we can of bogus pointers.

regards, tom lane

Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Tue, 6 Sept 2022 at 14:32, David Rowley  wrote:
> I wonder if there are many usages of MemoryContextContains in
> extensions. If there's not, I'd be much happier if we got rid of this
> function and used GetMemoryChunkContext() in nodeAgg.c and
> nodeWindowAgg.c.

I see postgis is one user of it, per [1].  The other extensions
mentioned there just seem to be copying code and not using it.

David

[1] https://codesearch.debian.net/search?q=MemoryContextContains=1

Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Tue, 6 Sept 2022 at 12:27, Tom Lane  wrote:
>
> David Rowley  writes:
> > On Tue, 6 Sept 2022 at 11:09, Andres Freund  wrote:
> >> I was looking at
> >> MemoryContextContains(). Unless I am missing something, the patch omitted
> >> adjusting that? We'll probably always return false right now.
>
> > Oops. Yes. I'll push a fix a bit later.

I think the fix is harder than I thought, or perhaps impossible to do
given how we now determine the owning MemoryContext of a pointer.

There's a comment in MemoryContextContains which says:

* NB: Can't use GetMemoryChunkContext() here - that performs assertions
* that aren't acceptable here since we might be passed memory not
* allocated by any memory context.

That seems to indicate that we should be able to handle any random
pointer given to us (!).  That comment seems more confident that'll
work than the function's header comment does:

 * Caution: this test is reliable as long as 'pointer' does point to
 * a chunk of memory allocated from *some* context.  If 'pointer' points
 * at memory obtained in some other way, there is a small chance of a
 * false-positive result, since the bits right before it might look like
 * a valid chunk header by chance.

Here that's just claiming the test might not be reliable and could
return false-positive results.

I find this entire function pretty scary as even before the context
changes that function seems to think it's fine to subtract sizeof(void
*) from the given pointer and dereference that memory. That could very
well segfault.

I wonder if there are many usages of MemoryContextContains in
extensions. If there's not, I'd be much happier if we got rid of this
function and used GetMemoryChunkContext() in nodeAgg.c and
nodeWindowAgg.c.

> +1 for adding something to regress.c that verifies that this
> works properly for all three allocators.  I suggest making
> three contexts and cross-checking the correct results for
> all combinations of chunk A vs context B.

I went as far as adding an Assert to palloc(). I'm not quite sure what
you have in mind in regress.c

Attached is a draft patch. I just don't like this function one bit.

David
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 64bcc7ef32..a6b2d02b75 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -809,11 +809,10 @@ MemoryContextCheck(MemoryContext context)
  * Detect whether an allocated chunk of memory belongs to a given
  * context or not.
  *
- * Caution: this test is reliable as long as 'pointer' does point to
- * a chunk of memory allocated from *some* context.  If 'pointer' points
- * at memory obtained in some other way, there is a small chance of a
- * false-positive result, since the bits right before it might look like
- * a valid chunk header by chance.
+ * Caution: 'pointer' must point to a pointer which was allocated by a
+ * MemoryContext.  It's not safe or valid to use this function on arbitrary
+ * pointers as obtaining the MemoryContext which 'pointer' belongs to requires
+ * possibly several pointer dereferences.
  */
 bool
 MemoryContextContains(MemoryContext context, void *pointer)
@@ -821,10 +820,6 @@ MemoryContextContains(MemoryContext context, void *pointer)
MemoryContext ptr_context;
 
/*
-* NB: Can't use GetMemoryChunkContext() here - that performs assertions
-* that aren't acceptable here since we might be passed memory not
-* allocated by any memory context.
-*
 * Try to detect bogus pointers handed to us, poorly though we can.
 * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
 * allocated chunk.
@@ -835,7 +830,7 @@ MemoryContextContains(MemoryContext context, void *pointer)
/*
 * OK, it's probably safe to look at the context.
 */
-   ptr_context = *(MemoryContext *) (((char *) pointer) - sizeof(void *));
+   ptr_context = GetMemoryChunkContext(pointer);
 
return ptr_context == context;
 }
@@ -1151,6 +1146,8 @@ palloc(Size size)
   size, context->name)));
}
 
+   Assert(MemoryContextContains(context, ret));
+
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
return ret;

Re: Remove dead macro exec_subplan_get_plan

[Richard Guo]

On Tue, Sep 6, 2022 at 1:18 AM Tom Lane  wrote:

> Zhang Mingli  writes:
> > Macro exec_subplan_get_plan is not used anymore.
> > Attach a patch to remove it.
>
> Hm, I wonder why it's not used anymore.  Maybe we no longer need
> that list at all?  If we do, should use of the macro be
> re-introduced in the accessors?


Seems nowadays no one fetches the Plan from PlannedStmt->subplans with a
certain plan_id any more. Previously back in eab6b8b2 where this macro
was introduced, it was used in explain_outNode and ExecInitSubPlan.

I find a similar macro, planner_subplan_get_plan, who fetches the Plan
from glob->subplans. We can use it in the codes where needed. For
example, in the new function SS_make_multiexprs_unique.

 /* Found one, get the associated subplan */
-plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1);
+plan = planner_subplan_get_plan(root, splan);

Thanks
Richard

Re: pg15b3: recovery fails with wal prefetch enabled

[Thomas Munro]

On Tue, Sep 6, 2022 at 1:51 PM Tom Lane  wrote:
> "Jonathan S. Katz"  writes:
> > On 9/5/22 7:18 PM, Thomas Munro wrote:
> >> Well I was about to commit this, but beta4 just got stamped (but not
> >> yet tagged).  I see now that Jonathan (with RMT hat on, CC'd) meant
> >> commits should be in by the *start* of the 5th AoE, not the end.  So
> >> the procedural/RMT question is whether it's still possible to close
> >> this item in beta4.
>
> > Presumably because Tom stamped it, the released is wrapped so it
> > wouldn't make Beta 4, but I defer to him to see if it can be included
> > with the tag.
>
> I already made the tarballs available to packagers, so adding this
> would involve a re-wrap and great confusion.  In any case, I'm not
> a fan of pushing fixes within a day or two of the wrap deadline,
> let alone after it; you get inadequate buildfarm coverage when you
> cut corners that way.  I think this one missed the boat.

Got it.  Yeah I knew it was going to be a close thing with a problem
diagnosed on Thursday/Friday before a Monday wrap, even before I
managed to confuse myself about dates and times.  Thanks both.

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[David Rowley]

On Tue, 6 Sept 2022 at 13:52, Ranier Vilela  wrote:
>
> Em seg., 5 de set. de 2022 às 22:29, David Rowley  
> escreveu:
>> It feels like it would be good if we had a way to detect a few of
>> these issues much earlier than we are currently.  There's been a long
>> series of commits fixing up this sort of thing.  If we had a tool to
>> parse the .c files and look for things like a function call to
>> appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
>> no va_arg arguments).
>
> StaticAssert could check va_arg no?

I'm not sure exactly what you have in mind. If you think you have a
way to make that work, it would be good to see a patch with it.

David

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[Ranier Vilela]

Em seg., 5 de set. de 2022 às 22:29, David Rowley 
escreveu:

> On Tue, 6 Sept 2022 at 06:07, Ranier Vilela  wrote:
> > I did a search and found a few more places.
> > v1 attached.
>
> Thanks.  I've done a bit more looking and found a few more places that
> we can improve and I've pushed the result.
>
Thanks.


>
> It feels like it would be good if we had a way to detect a few of
> these issues much earlier than we are currently.  There's been a long
> series of commits fixing up this sort of thing.  If we had a tool to
> parse the .c files and look for things like a function call to
> appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
> no va_arg arguments).
>
StaticAssert could check va_arg no?

regards,
Ranier Vilela

Re: pg15b3: recovery fails with wal prefetch enabled

[Tom Lane]

"Jonathan S. Katz"  writes:
> On 9/5/22 7:18 PM, Thomas Munro wrote:
>> Well I was about to commit this, but beta4 just got stamped (but not
>> yet tagged).  I see now that Jonathan (with RMT hat on, CC'd) meant
>> commits should be in by the *start* of the 5th AoE, not the end.  So
>> the procedural/RMT question is whether it's still possible to close
>> this item in beta4.

> Presumably because Tom stamped it, the released is wrapped so it 
> wouldn't make Beta 4, but I defer to him to see if it can be included 
> with the tag.

I already made the tarballs available to packagers, so adding this
would involve a re-wrap and great confusion.  In any case, I'm not
a fan of pushing fixes within a day or two of the wrap deadline,
let alone after it; you get inadequate buildfarm coverage when you
cut corners that way.  I think this one missed the boat.

regards, tom lane

Re: pg15b3: recovery fails with wal prefetch enabled

[Jonathan S. Katz]

On 9/5/22 7:18 PM, Thomas Munro wrote:

On Mon, Sep 5, 2022 at 9:08 PM Thomas Munro  wrote:

At Mon, 05 Sep 2022 14:15:27 +0900 (JST), Kyotaro Horiguchi 
 wrote in
At Mon, 5 Sep 2022 16:54:07 +1200, Thomas Munro  wrote 
in

On reflection, it'd be better not to clobber any pre-existing error
there, but report one only if there isn't one already queued.  I've
done that in this version, which I'm planning to do a bit more testing
on and commit soonish if there are no comments/objections, especially
for that part.


Well I was about to commit this, but beta4 just got stamped (but not
yet tagged).  I see now that Jonathan (with RMT hat on, CC'd) meant
commits should be in by the *start* of the 5th AoE, not the end.  So
the procedural/RMT question is whether it's still possible to close
this item in beta4.


Presumably because Tom stamped it, the released is wrapped so it 
wouldn't make Beta 4, but I defer to him to see if it can be included 
with the tag.


That said, if it doesn't make it for Beta 4, it would be in the next 
release (which is hopefully RC1).


Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[David Rowley]

On Tue, 6 Sept 2022 at 06:07, Ranier Vilela  wrote:
> I did a search and found a few more places.
> v1 attached.

Thanks.  I've done a bit more looking and found a few more places that
we can improve and I've pushed the result.

It feels like it would be good if we had a way to detect a few of
these issues much earlier than we are currently.  There's been a long
series of commits fixing up this sort of thing.  If we had a tool to
parse the .c files and look for things like a function call to
appendPQExpBuffer() and appendStringInfo() with only 2 parameters (i.e
no va_arg arguments).

I'll hold off a few days before pushing the other patch.  Tom stamped
beta4 earlier, so I'll hold off until after the tag.

David

Re: [PATCH] Tab completion for SET COMPRESSION

[Shinya Kato]

On 2022-08-22 21:48, Aleksander Alekseev wrote:

Hi hackers,

The proposed patch adds the missing tab completion for 'ALTER TABLE
... SET COMPRESSION ...' syntax.

Thanks, LGTM.

In addition, why not take this opportunity to create a tab completion 
for "ALTER TABLE  OF " and "ALTER TABLE  NOT OF"?



--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: pg15b3: recovery fails with wal prefetch enabled

[Kyotaro Horiguchi]

At Mon, 5 Sep 2022 21:08:16 +1200, Thomas Munro  wrote 
in 
> We also need the LSN that is past that record.
> XLogReleasePreviousRecord() could return it (or we could use
> reader->EndRecPtr I suppose).  Thoughts on this version?

(Catching the gap...)

It is easier to read. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Reducing the chunk header sizes on all memory context types

[Tom Lane]

David Rowley  writes:
> On Tue, 6 Sept 2022 at 11:09, Andres Freund  wrote:
>> I was looking at
>> MemoryContextContains(). Unless I am missing something, the patch omitted
>> adjusting that? We'll probably always return false right now.

> Oops. Yes. I'll push a fix a bit later.

The existing uses in nodeAgg and nodeWindowAgg failed to expose this
because an incorrect false result just causes them to do extra work
(ie, a useless datumCopy).  I think there might be a memory leak
too, but the regression tests wouldn't run an aggregation long
enough to make that obvious either.

+1 for adding something to regress.c that verifies that this
works properly for all three allocators.  I suggest making
three contexts and cross-checking the correct results for
all combinations of chunk A vs context B.

regards, tom lane

Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Tue, 6 Sept 2022 at 11:09, Andres Freund  wrote:
> I was looking at
> MemoryContextContains(). Unless I am missing something, the patch omitted
> adjusting that? We'll probably always return false right now.

Oops. Yes. I'll push a fix a bit later.

David

Re: Mingw task for Cirrus CI

[Andres Freund]

Hi,

On 2022-09-05 06:50:55 -0500, Justin Pryzby wrote:
> I saw that but hadn't tracked it down yet.  Do you know if the tar
> failures were from a TAP test added since you first posted the mingw
> patch, or ??

I think it's that msys now includes tar by default, but not sure.


> Also: your original patch said --host=x86_64-w64-mingw32, but the task
> is called MinGW64.  The most recent patches don't use --host at all, and
> were building for a 32 bit environment, even though the OS image says
> MSYSTEM=UCRT64.

I don't think you should need to use --host, that indicates cross compiling,
which we shouldn't do. I don't think the patch without --host targets 32bit -
the last time CI ran the patch, it built a 64bit PG:

https://cirrus-ci.com/task/5489471041830912?logs=configure#L231
[13:47:54.539] checking size of void *... (cached) 8
[13:47:54.557] checking size of size_t... (cached) 8

and the underlying commit didn't specify --host.


> Also: right now src/test and src/interfaces/*/test aren't being built
> during the build phase, which means that they're 1) not compiled in
> parallel; and 2) not cached.  This isn't specific to MinGW.  Other than
> compiling those dirs specifically, one option is to put
> "always: upload_caches: ccache" after "test_world_script" (in that case,
> if the CI instance is rescheduled during tests, the compilation won't be
> pushed to cache).  Actually, it seems better to compile stuff during
> "build" or else any compilation warnings should up in the middle of
> "check-world.."

I'd tackle that independently of this commit.


> Also: I'm having second thoughts about the loop around ./configure.  It
> could happen that a cached configure would succeed, but then the build
> would later fail, and it wouldn't fix itself.  I think a slow configure
> is okay for an "opt-in" task.

Agreed.

I think we can convert this to meson soon, and that seems a *lot* faster at
configure than autoconf on mingw. Not even close to as fast as on a modern-ish
linux, but not that painful.


Greetings,

Andres Freund

Re: Modernizing our GUC infrastructure

[Tom Lane]

Andres Freund  writes:
> It's only half related, but since we're talking about renovating guc.c: I
> think it'd be good if we split the list of GUCs from the rest of the guc
> machinery. Both for humans and compilers it's getting pretty large. And
> commonly one either wants to edit the definition of GUCs or wants to edit the
> GUC machinery.

I don't mind doing that, but it seems like an independent patch.

regards, tom lane

Re: Column Filtering in Logical Replication

[Peter Smith]

On Mon, Sep 5, 2022 at 8:46 PM Amit Kapila  wrote:
>
> On Mon, Sep 5, 2022 at 3:46 PM Peter Smith  wrote:
> >
> >
> > PSA v7.
> >
>
> For example, if additional columns are added to the table, then
> +(after a REFRESH PUBLICATION) if there was a column 
> list
> +only those named columns will continue to be replicated.
>
> This looks a bit unclear to me w.r.t the refresh publication step. Why
> exactly you have used refresh publication in the above para? It is
> used to add new tables if any added to the publication, so not clear
> to me how it helps in this case. If that is not required then we can
> change it to: "For example, if additional columns are added to the
> table then only those named columns mentioned in the column list will
> continue to be replicated."
>

You are right - that REFRESH PUBLICATION was not necessary for this
example. The patch is modified to use your suggested text.

PSA v8

--
Kind Regards,
Peter Smith.
Fujitsu Australia


v8-0001-Column-List-new-pgdocs-section.patch
Description: Binary data

Re: Modernizing our GUC infrastructure

[Andres Freund]

Hi,

> I wrote this because I am starting to question the schema-variables patch
> [1] --- that's getting to be quite a large patch and I grow less and less
> sure that it's solving a problem our users want solved. --- that's getting
> to be quite a large patch and I grow less and less sure that it's solving a
> problem our users want solved.  I think what people actually want is better
> support of the existing mechanism for ad-hoc session variables, namely
> abusing custom GUCs for that purpose.

I don't really have an opinion on the highlevel directional question, yet
anyway. But the stuff you're talking about changing in guc.c seem like a good
idea independently.


On 2022-09-05 18:27:46 -0400, Tom Lane wrote:
> 0001 and 0002 below are concerned with converting guc.c to store its
> data in a dedicated memory context (GUCMemoryContext) instead of using
> raw malloc().  This is not directly a performance improvement, and
> I'm prepared to drop the idea if there's a lot of pushback, but I
> think it would be a good thing to do.

+1 - I've been annoyed at this a couple times, even just because it makes it
harder to identify memory leaks etc.


> The only hard reason for using
> malloc() there was the lack of ability to avoid throwing elog(ERROR)
> on out-of-memory in palloc().  But mcxt.c grew that ability years ago.
> Switching to a dedicated context would greatly improve visibility and
> accountability of GUC-related allocations.  Also, the 0003 patch will
> switch guc.c to relying on a palloc-based hashtable, and it seems a
> bit silly to have part of the data structure in palloc and part in
> malloc.  However 0002 is a bit invasive, in that it forces code
> changes in GUC check callbacks, if they want to reallocate the new
> value or create an "extra" data structure.  My feeling is that not
> enough external modules use those facilities for this to pose a big
> problem.  However, the ones that are subject to it will have a
> non-fun time tracking down why their code is crashing.

That sucks, but I think it's a bullet we're going to have to bite at some
point.

Perhaps we could do something like checking MemoryContextContains() and assert
if not allocated in the right context? That way the crash is at least
obvious. Or perhaps even transparently reallocate in that case?  It does look
like MemoryContextContains() currently is broken, I've raised that in the
other thread.


> (The recent context-header changes mean that you get a very obscure failure
> when trying to pfree() a malloc'd chunk -- for me, that typically ends in an
> assertion failure in generation.c.  Can we make that less confusing?)

Hm. We can do better in assert builds, but I'm not sure we want to add the
overhead of explicit checks in normal builds, IIRC David measured the overhead
of additional branches in pfree, and it was noticable.


> 0003 replaces guc.c's bsearch-a-sorted-array lookup infrastructure
> with a dynahash hash table.  (I also looked at simplehash, but it
> has no support for not elog'ing on OOM, and it increases the size
> of guc.o by 10KB or so.)

Dynahash seems reasonable here. Hard to believe raw lookup speed is a relevant
bottleneck and due to the string names the key would be pretty wide (could
obviously just be done via pointer, but then the locality benefits aren't as
big).


> However, the places that were linearly scanning the array now need to use
> hash_seq_search, so some other things like transaction shutdown
> (AtEOXact_GUC) get slower.
>
> To address that, 0004 adds some auxiliary lists that link together
> just the variables that are interesting for specific purposes.

Seems sane.


It's only half related, but since we're talking about renovating guc.c: I
think it'd be good if we split the list of GUCs from the rest of the guc
machinery. Both for humans and compilers it's getting pretty large. And
commonly one either wants to edit the definition of GUCs or wants to edit the
GUC machinery.


Greetings,

Andres Freund

Re: pg15b3: recovery fails with wal prefetch enabled

[Thomas Munro]

On Mon, Sep 5, 2022 at 9:08 PM Thomas Munro  wrote:
> > At Mon, 05 Sep 2022 14:15:27 +0900 (JST), Kyotaro Horiguchi 
> >  wrote in
> > At Mon, 5 Sep 2022 16:54:07 +1200, Thomas Munro  
> > wrote in
> > > On reflection, it'd be better not to clobber any pre-existing error
> > > there, but report one only if there isn't one already queued.  I've
> > > done that in this version, which I'm planning to do a bit more testing
> > > on and commit soonish if there are no comments/objections, especially
> > > for that part.

Well I was about to commit this, but beta4 just got stamped (but not
yet tagged).  I see now that Jonathan (with RMT hat on, CC'd) meant
commits should be in by the *start* of the 5th AoE, not the end.  So
the procedural/RMT question is whether it's still possible to close
this item in beta4.

Re: Reducing the chunk header sizes on all memory context types

[Andres Freund]

Hi,

On 2022-08-29 17:26:29 +1200, David Rowley wrote:
> On Mon, 29 Aug 2022 at 10:39, David Rowley  wrote:
> > One more try to make CFbot happy.
>
> After a bit more revision, mostly updating outdated comments and
> naming adjustments, I've pushed this.

Responding to Tom's email about guc.c changes [1], I was looking at
MemoryContextContains(). Unless I am missing something, the patch omitted
adjusting that? We'll probably always return false right now.

Probably should have something that tests that MemoryContextContains() works
at least to some degree. Perhaps a test in regress.c?

Greetings,

Andres Freund

[1] https://postgr.es/m/2982579.1662416866%40sss.pgh.pa.us

Re: Patch to address creation of PgStat* contexts with null parent context

[Andres Freund]

Hi,

On 2022-09-05 17:32:20 +0900, Kyotaro Horiguchi wrote:
> The rationale of creating them at pgstat_attach_shmem is that anyway once
> pgstat_attach_shmem is called, the process fainally creates the contexts at
> the end of the process, and (I think) it's simpler that we don't do if()
> check at every pgstat_get_entry_ref() call.

But that's not true, as pointed out here:
https://postgr.es/m/20220808192020.nc556tlgcp66fdgw%40awork3.anarazel.de

Nor does it make sense to reserve memory for the entire lifetime of a process
just because we might need it for a split second at the end.

Greetings,

Andres Freund

Modernizing our GUC infrastructure

[Tom Lane]

Attached is a patch series that attempts to modernize our GUC
infrastructure, in particular removing the performance bottlenecks
it has when there are lots of GUC variables.  I wrote this because
I am starting to question the schema-variables patch [1] --- that's
getting to be quite a large patch and I grow less and less sure
that it's solving a problem our users want solved.  I think what
people actually want is better support of the existing mechanism
for ad-hoc session variables, namely abusing custom GUCs for that
purpose.  One of the big reasons we have been resistant to formally
supporting that is fear of the poor performance guc.c would have
with lots of variables.  But we already have quite a lot of them:

regression=# select count(*) from pg_settings;
 count 
---
   353
(1 row)

and more are getting added all the time.  I think this patch series
could likely be justified just in terms of positive effect on core
performance, never mind user-added GUCs.

0001 and 0002 below are concerned with converting guc.c to store its
data in a dedicated memory context (GUCMemoryContext) instead of using
raw malloc().  This is not directly a performance improvement, and
I'm prepared to drop the idea if there's a lot of pushback, but I
think it would be a good thing to do.  The only hard reason for using
malloc() there was the lack of ability to avoid throwing elog(ERROR)
on out-of-memory in palloc().  But mcxt.c grew that ability years ago.
Switching to a dedicated context would greatly improve visibility and
accountability of GUC-related allocations.  Also, the 0003 patch will
switch guc.c to relying on a palloc-based hashtable, and it seems a
bit silly to have part of the data structure in palloc and part in
malloc.  However 0002 is a bit invasive, in that it forces code
changes in GUC check callbacks, if they want to reallocate the new
value or create an "extra" data structure.  My feeling is that not
enough external modules use those facilities for this to pose a big
problem.  However, the ones that are subject to it will have a
non-fun time tracking down why their code is crashing.  (The recent
context-header changes mean that you get a very obscure failure when
trying to pfree() a malloc'd chunk -- for me, that typically ends
in an assertion failure in generation.c.  Can we make that less
confusing?)

0003 replaces guc.c's bsearch-a-sorted-array lookup infrastructure
with a dynahash hash table.  (I also looked at simplehash, but it
has no support for not elog'ing on OOM, and it increases the size
of guc.o by 10KB or so.)  This fixes the worse-than-O(N^2) time
needed to create N new GUCs, as in

do $$
begin
  for i in 1..1 loop
perform set_config('foo.bar' || i::text, i::text, false);
  end loop;
end $$;

On my machine, this takes about 4700 ms in HEAD, versus 23 ms
with this patch.  However, the places that were linearly scanning
the array now need to use hash_seq_search, so some other things
like transaction shutdown (AtEOXact_GUC) get slower.

To address that, 0004 adds some auxiliary lists that link together
just the variables that are interesting for specific purposes.
This is helpful even without considering the possibility of a
lot of user-added GUCs: in a typical session, for example, not
many of those 353 GUCs have non-default values, and even fewer
get modified in any one transaction (typically, anyway).

As an example of the speedup from 0004, these DO loops:

create or replace function func_with_set(int) returns int
strict immutable language plpgsql as
$$ begin return $1; end $$
set enable_seqscan = false;

do $$
begin
  for i in 1..10 loop
perform func_with_set(i);
  end loop;
end $$;

do $$
begin
  for i in 1..10 loop
begin
  perform func_with_set(i);
exception when others then raise;
end;
  end loop;
end $$;

take about 260 and 320 ms respectively for me, in HEAD with
just the stock set of variables.  But after creating 1
GUCs with the previous DO loop, they're up to about 3200 ms.
0004 brings that back down to being indistinguishable from the
speed with few GUCs.

So I think this is good cleanup in its own right, plus it
removes one major objection to considering user-defined GUCs
as a supported feature.

regards, tom lane

[1] 
https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com

commit ecaef8ecf5705b9638f81d2dfb300b57589fbb8a
Author: Tom Lane 
Date:   Mon Sep 5 16:14:41 2022 -0400

Preliminary improvements in memory-context infrastructure.

We lack a version of repalloc() that supports MCXT_ALLOC_NO_OOM
semantics, so invent repalloc_extended().  repalloc_huge()
becomes a legacy wrapper for that.

Also, fix dynahash.c so that it can support HASH_ENTER_NULL
requests when using the default palloc-based allocator.
The only reason it was like that was the lack of the
MCXT_ALLOC_NO_OOM option when that code was written, ages 

Re: failing to build preproc.c on solaris with sun studio

[Tom Lane]

Peter Eisentraut  writes:
> Why is this being proposed?

Andres is annoyed by the long build time of ecpg, which he has to
wait for whether he wants to test it or not.  I could imagine that
I might disable ecpg testing on my slowest buildfarm animals, too.

I suppose maybe we could compromise on inventing --with-ecpg but
having it default to "on", so that you have to take positive
action if you don't want it.

regards, tom lane

Re: failing to build preproc.c on solaris with sun studio

[Andres Freund]

Hi,

On 2022-09-05 22:52:03 +0200, Peter Eisentraut wrote:
> On 04.09.22 16:55, Tom Lane wrote:
> > I guess we could proceed like this:
> > 
> > 1. Invent the --with option.  Temporarily make "make check" in ecpg
> > print a message but not fail if the option wasn't selected.
> > 
> > 2. Update buildfarm client to recognize the option and skip ecpg-check
> > if not selected.
> > 
> > 3. Sometime down the road, after everyone's updated their buildfarm
> > animals, flip ecpg "make check" to throw an error reporting that
> > ecpg wasn't built.
> 
> Why is this being proposed?

On slower machines / certain platforms it's the bottleneck during compilation
(as e.g. evidenced in this thread). There's no proper way to run check-world
exempting ecpg. Most changes don't involve ecpg in any way, so having every
developer build preproc.o etc isn't necessary.

Greetings,

Andres Freund

Re: failing to build preproc.c on solaris with sun studio

[Peter Eisentraut]

On 04.09.22 16:55, Tom Lane wrote:

I guess we could proceed like this:

1. Invent the --with option.  Temporarily make "make check" in ecpg
print a message but not fail if the option wasn't selected.

2. Update buildfarm client to recognize the option and skip ecpg-check
if not selected.

3. Sometime down the road, after everyone's updated their buildfarm
animals, flip ecpg "make check" to throw an error reporting that
ecpg wasn't built.


Why is this being proposed?

Re: Instrumented pages/tuples frozen in autovacuum's server log out (and VACUUM VERBOSE)

[Peter Geoghegan]

On Wed, Aug 31, 2022 at 7:49 PM Jeff Janes  wrote:
> I think "frozen:" would be a more suitable line prefix.

Attached revision does it that way.

Barring any objections I will commit this patch within the next few days.

Thanks
-- 
Peter Geoghegan


v2-0001-Instrument-freezing-in-autovacuum-log-reports.patch
Description: Binary data

Re: Table AM modifications to accept column projection lists

[Zhihong Yu]

On Mon, Sep 5, 2022 at 9:51 AM Nikita Malakhov  wrote:

> Hi hackers!
>
> This is the original patch rebased onto v15 master with conflicts
> resolved. I'm currently
> studying it and latest comments in the original thread, and would try go
> the way that
> was mentioned in the thread (last message) -
> [1]
> https://stratos.seas.harvard.edu/files/stratos/files/columnstoresfntdbs.pdf
> [2] https://github.com/zhihuiFan/postgres/tree/lazy_material_v2
> I agree it is not in the state for review, so I've decided not to change
> patch status,
> just revive the thread because we found that Pluggable Storage API is not
> somewhat
> not sufficient.
> Thanks for the recommendations, I'll check that out.
>
> Hi,
bq. is not somewhat not sufficient.

I am a bit confused by the double negation.
I guess you meant insufficient.

Cheers

Re: predefined role(s) for VACUUM and ANALYZE

[Nathan Bossart]

Here is a first attempt at allowing users to grant VACUUM or ANALYZE
per-relation.  Overall, this seems pretty straightforward.  I needed to
adjust the permissions logic for VACUUM/ANALYZE a bit, which causes some
extra WARNING messages for VACUUM (ANALYZE) in some cases, but this didn't
seem particularly worrisome.  It may be desirable to allow granting ANALYZE
on specific columns or to allow granting VACUUM/ANALYZE at the schema or
database level, but that is left as a future exercise.

On Tue, Aug 23, 2022 at 07:46:47PM -0400, Stephen Frost wrote:
> I've long felt that we should redefine the way the ACLs work to have a
> distinct set of bits for each object type.  We don't need to support a
> CONNECT bit on a table, yet we do today and we expend quite a few bits
> in that way.  Having that handled on a per-object-type basis instead
> would allow us to get quite a bit more mileage out of the existing 32bit
> field before having to introduce more complicated storage methods like
> using a bit to tell us to go look up more ACLs somewhere else.

There are 2 bits remaining at the moment, so I didn't redesign the ACL
system in the attached patch.  However, I did some research on a couple
options.  Using a distinct set of bits for each catalog table should free
up a handful of bits, which should indeed kick the can down the road a
little.  Another easy option is to simply make AclMode a uint64, which
would immediately free up another 16 privilege bits.  I was able to get
this approach building and passing tests in a few minutes, but there might
be performance/space concerns.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f19eea3f3148916a79d002094ca4eb4aa98af753 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Sat, 3 Sep 2022 23:31:38 -0700
Subject: [PATCH v3 1/1] Allow granting VACUUM and ANALYZE privileges on
 relations.

---
 doc/src/sgml/ddl.sgml | 49 ---
 doc/src/sgml/func.sgml|  3 +-
 .../sgml/ref/alter_default_privileges.sgml|  4 +-
 doc/src/sgml/ref/analyze.sgml |  3 +-
 doc/src/sgml/ref/grant.sgml   |  4 +-
 doc/src/sgml/ref/revoke.sgml  |  2 +-
 doc/src/sgml/ref/vacuum.sgml  |  3 +-
 src/backend/catalog/aclchk.c  |  8 ++
 src/backend/commands/analyze.c|  2 +-
 src/backend/commands/vacuum.c | 24 --
 src/backend/parser/gram.y |  7 ++
 src/backend/utils/adt/acl.c   | 16 
 src/bin/pg_dump/dumputils.c   |  2 +
 src/bin/pg_dump/t/002_pg_dump.pl  |  2 +-
 src/bin/psql/tab-complete.c   |  4 +-
 src/include/nodes/parsenodes.h|  4 +-
 src/include/utils/acl.h   |  6 +-
 src/test/regress/expected/dependency.out  | 22 ++---
 src/test/regress/expected/privileges.out  | 86 ++-
 src/test/regress/expected/rowsecurity.out | 34 
 src/test/regress/expected/vacuum.out  |  6 ++
 src/test/regress/sql/dependency.sql   |  2 +-
 src/test/regress/sql/privileges.sql   | 40 +
 23 files changed, 249 insertions(+), 84 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 03c0193709..ed034a6b1d 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1691,8 +1691,9 @@ ALTER TABLE products RENAME TO items;
INSERT, UPDATE, DELETE,
TRUNCATE, REFERENCES, TRIGGER,
CREATE, CONNECT, TEMPORARY,
-   EXECUTE, USAGE, SET
-   and ALTER SYSTEM.
+   EXECUTE, USAGE, SET,
+   ALTER SYSTEM, VACUUM, and
+   ANALYZE.
The privileges applicable to a particular
object vary depending on the object's type (table, function, etc.).
More detail about the meanings of these privileges appears below.
@@ -1982,7 +1983,25 @@ REVOKE ALL ON accounts FROM PUBLIC;
   
  
 
-   
+
+   
+VACUUM
+
+ 
+  Allows VACUUM on a relation.
+ 
+
+   
+
+   
+ANALYZE
+
+ 
+  Allows ANALYZE on a relation.
+ 
+
+   
+  
 
The privileges required by other commands are listed on the
reference page of the respective command.
@@ -2131,6 +2150,16 @@ REVOKE ALL ON accounts FROM PUBLIC;
   A
   PARAMETER
  
+ 
+  VACUUM
+  v
+  TABLE
+ 
+ 
+  ANALYZE
+  z
+  TABLE
+ 
  

   
@@ -2221,7 +2250,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
  
  
   TABLE (and table-like objects)
-  arwdDxt
+  arwdDxtvz
   none
   \dp
  
@@ -2279,12 +2308,12 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
would show:
 
 = \dp mytable
-  Access privileges
- Schema |  Name   | Type  |   Access privileges   |   Column privileges   | Policies
-+-+---+---+---+--
- public | mytable | table | 

Re: Backpatching nbtree VACUUM (page deletion) hardening

[Peter Geoghegan]

On Fri, Sep 2, 2022 at 6:51 PM Peter Geoghegan  wrote:
> Yes -- nbtree VACUUM generally can cope quite well, even when the
> index is corrupt. It should mostly manage to do what is expected here,
> even with a misbehaving opclass, because it relies as little as
> possible on user-defined opclass code.

I just backpatched the hardening commit from 14 to every supported branch.

-- 
Peter Geoghegan

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[Ranier Vilela]

Em seg., 5 de set. de 2022 às 10:40, David Rowley 
escreveu:

> On Mon, 5 Sept 2022 at 22:15, David Rowley  wrote:
> > On Sat, 3 Sept 2022 at 00:37, Ranier Vilela  wrote:
> > > 6. Avoid overhead when using unnecessary StringInfoData to convert
> Datum a to Text b.
> >
> > I've ripped out #4 and #6 for now. I think we should do #6 in master
> > only, probably as part of a wider cleanup of StringInfo misusages.
>
> I've attached a patch which does various other string operation cleanups.
>
> * This changes cstring_to_text() to use cstring_to_text_with_len when
> we're working with a StringInfo and can just access the .len field.
> * Uses appendStringInfoString instead of appendStringInfo when there
> is special formatting.
> * Uses pstrdup(str) instead of psprintf("%s", str).  In many cases
> this will save a bit of memory
> * Uses appendPQExpBufferChar instead of appendPQExpBufferStr() when
> appending a 1 byte string.
> * Uses appendStringInfoChar() instead of appendStringInfo() when no
> formatting and string is 1 byte.
> * Uses appendStringInfoChar() instead of appendStringInfoString() when
> string is 1 byte.
> * Uses appendPQExpBuffer(b , ...) instead of appendPQExpBufferStr(b, "%s"
> ...)
>
> I'm aware there are a few other places that we could use
> cstring_to_text_with_len() instead of cstring_to_text(). For example,
> using the return value of snprintf() to obtain the length. I just
> didn't do that because we need to take care to check the return value
> isn't -1.
>
> My grep patterns didn't account for these function calls spanning
> multiple lines, so I may have missed a few.
>
I did a search and found a few more places.
v1 attached.

regards,
Ranier Vilela


v1-string_fixes.patch
Description: Binary data

Re: doc: mentioned CREATE+ATTACH PARTITION as an alternative to CREATE TABLE..PARTITION OF

[Justin Pryzby]

On Thu, Aug 04, 2022 at 01:45:49AM -0400, Robert Treat wrote:
> After reading this again, it isn't clear to me that this advice would
> be more appropriately placed into Section 5.11, aka
> https://www.postgresql.org/docs/current/ddl-partitioning.html, but in
> lieu of a specific suggestion for where to place it there (I haven't
> settled on one yet), IMHO, I think the first sentence of the suggested
> change should be rewritten as:
> 
> 
> Note that creating a partition using PARTITION OF
> requires taking an ACCESS EXCLUSIVE lock on the parent 
> table.
> It may be preferable to first CREATE a separate table...

Thanks for looking.  I used your language.

There is some relevant information in ddl.sgml, but not a lot, and it's
not easily referred to, so I removed the part of the patch that tried to
cross-reference.

@Robert: I wonder why shouldn't CREATE..PARTITION OF *also* be patched
to first create a table, and then attach the partition, transparently
doing what everyone would want, without having to re-read the updated
docs or know to issue two commands?  I wrote a patch for this which
"doesn't fail tests", but I still wonder if I'm missing something..

commit 723fa7df82f39aed5d58e5e52ba80caa8cb13515
Author: Justin Pryzby 
Date:   Mon Jul 18 09:24:55 2022 -0500

doc: mention CREATE+ATTACH PARTITION as an alternative to CREATE 
TABLE..PARTITION OF

In v12, 898e5e329 (Allow ATTACH PARTITION with only 
ShareUpdateExclusiveLock)
allows attaching a partition with a weaker lock than in CREATE..PARTITION 
OF,
but it does that silently.  On the one hand, things that are automatically
better, without having to enable the option are the best kind of feature.

OTOH, I doubt many people know to do that, because the docs don't say
so, because it was implemented as an transparent improvement.  This
patch adds a bit of documentations to make that more visible.

See also: 898e5e3290a72d288923260143930fb32036c00c
Should backpatch to v12

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 360284e37d6..66138b9299d 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4092,7 +4092,9 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
 
 
  The ATTACH PARTITION command requires taking a
- SHARE UPDATE EXCLUSIVE lock on the partitioned table.
+ SHARE UPDATE EXCLUSIVE lock on the partitioned table,
+ as opposed to the Access Exclusive lock which is
+ required by CREATE TABLE .. PARTITION OF.
 
 
 
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index c14b2010d81..54dbfa72e4c 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -619,6 +619,16 @@ WITH ( MODULUS numeric_literal, REM
   with DROP TABLE requires taking an ACCESS
   EXCLUSIVE lock on the parent table.
  
+
+ 
+  Note that creating a partition using PARTITION OF
+  requires taking an ACCESS EXCLUSIVE lock on the parent
+  table.  It may be preferable to first create a separate table and then
+  attach it, which does not require as strong a lock.
+  See ATTACH 
PARTITION
+  for more information.
+ 
+
 

 

Re: Remove dead macro exec_subplan_get_plan

[Tom Lane]

Zhang Mingli  writes:
> Macro exec_subplan_get_plan is not used anymore.
> Attach a patch to remove it.

Hm, I wonder why it's not used anymore.  Maybe we no longer need
that list at all?  If we do, should use of the macro be
re-introduced in the accessors?

regards, tom lane

Re: pg_upgrade allows itself to be run twice

[Justin Pryzby]

rebased and updated

Robert thought that it might be reasonable for someone to initdb, and
then connect and make some modifications, and then pg_upgrade.
https://www.postgresql.org/message-id/CA%2BTgmoYwaXh_wRRa2CqL4XpM4r6YEbq1%2Bec%3D%2B8b7Dr7-T_tT%2BQ%40mail.gmail.com

But the DBs are dropped by pg_upgrade, so that seems to serve no
purpose, except for shared relations (and global objects?).  In the case
of shared relations, it seems unsafe (even though my test didn't cause
an immediate explosion).

So rather than continuing to allow arbitrary changes between initdb and
pg_upgrade, I propose to prohibit all changes.  I'd consider allowing an
"inclusive" list of allowable changes, if someone were to propose such a
thing - but since DBs are dropped, I'm not sure what it could include.
>From 7958ec4b13e9dc581f69df65933789274de499b4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 6 Jul 2022 08:55:11 -0500
Subject: [PATCH v2] wip: initdb should advance the OID counter to FirstNormal

Otherwise, a cluster started in single-user mode immediately after
initdb can create objects in the range of system OIDs, and pg_upgrade
might be able to be run twice (if the cluster has no relations/objects).
---
 src/backend/access/transam/varsup.c |  5 ++---
 src/bin/initdb/initdb.c |  9 +
 src/bin/pg_upgrade/check.c  |  5 +
 src/bin/pg_upgrade/pg_upgrade.c | 13 -
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 849a7ce9d6d..d93974fcaa5 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -543,8 +543,7 @@ GetNewObjectId(void)
 	 *
 	 * During initdb, we start the OID generator at FirstGenbkiObjectId, so we
 	 * only wrap if before that point when in bootstrap or standalone mode.
-	 * The first time through this routine after normal postmaster start, the
-	 * counter will be forced up to FirstNormalObjectId.  This mechanism
+	 * This mechanism
 	 * leaves the OIDs between FirstGenbkiObjectId and FirstNormalObjectId
 	 * available for automatic assignment during initdb, while ensuring they
 	 * will never conflict with user-assigned OIDs.
@@ -553,7 +552,7 @@ GetNewObjectId(void)
 	{
 		if (IsPostmasterEnvironment)
 		{
-			/* wraparound, or first post-initdb assignment, in normal mode */
+			/* wraparound */
 			ShmemVariableCache->nextOid = FirstNormalObjectId;
 			ShmemVariableCache->oidCount = 0;
 		}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index e00837ecacf..1aad2b335b8 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -61,6 +61,7 @@
 #include "sys/mman.h"
 #endif
 
+#include "access/transam.h"
 #include "access/xlog_internal.h"
 #include "catalog/pg_authid_d.h"
 #include "catalog/pg_class_d.h" /* pgrminclude ignore */
@@ -2735,6 +2736,14 @@ initialize_data_directory(void)
 
 	PG_CMD_CLOSE;
 
+	/* Set FirstNormal OID */
+	snprintf(cmd, sizeof(cmd),
+			 "\"%s/pg_resetwal\" -o %u %s",
+			 bin_path, FirstNormalObjectId, pg_data);
+
+	PG_CMD_OPEN;
+	PG_CMD_CLOSE;
+
 	check_ok();
 }
 
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index f4969bcdad7..f7ff9b5f65b 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -9,6 +9,7 @@
 
 #include "postgres_fe.h"
 
+#include "access/transam.h"
 #include "catalog/pg_authid_d.h"
 #include "catalog/pg_collation.h"
 #include "fe_utils/string_utils.h"
@@ -434,6 +435,10 @@ check_new_cluster_is_empty(void)
 {
 	int			dbnum;
 
+	if (new_cluster.controldata.chkpnt_nxtoid != FirstNormalObjectId)
+		pg_fatal("New cluster is not pristine: OIDs have been assigned since initdb (%u != %u)\n",
+			new_cluster.controldata.chkpnt_nxtoid, FirstNormalObjectId);
+
 	for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
 	{
 		int			relnum;
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 115faa222e3..ea7d93d9cbe 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -172,19 +172,6 @@ main(int argc, char **argv)
 	transfer_all_new_tablespaces(_cluster.dbarr, _cluster.dbarr,
  old_cluster.pgdata, new_cluster.pgdata);
 
-	/*
-	 * Assuming OIDs are only used in system tables, there is no need to
-	 * restore the OID counter because we have not transferred any OIDs from
-	 * the old system, but we do it anyway just in case.  We do it late here
-	 * because there is no need to have the schema load use new oids.
-	 */
-	prep_status("Setting next OID for new cluster");
-	exec_prog(UTILITY_LOG_FILE, NULL, true, true,
-			  "\"%s/pg_resetwal\" -o %u \"%s\"",
-			  new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
-			  new_cluster.pgdata);
-	check_ok();
-
 	if (user_opts.do_sync)
 	{
 		prep_status("Sync data directory to disk");
-- 
2.17.1

Re: Table AM modifications to accept column projection lists

[Nikita Malakhov]

Hi hackers!

This is the original patch rebased onto v15 master with conflicts resolved.
I'm currently
studying it and latest comments in the original thread, and would try go
the way that
was mentioned in the thread (last message) -
[1]
https://stratos.seas.harvard.edu/files/stratos/files/columnstoresfntdbs.pdf
[2] https://github.com/zhihuiFan/postgres/tree/lazy_material_v2
I agree it is not in the state for review, so I've decided not to change
patch status,
just revive the thread because we found that Pluggable Storage API is not
somewhat
not sufficient.
Thanks for the recommendations, I'll check that out.

On Mon, Sep 5, 2022 at 7:36 PM Justin Pryzby  wrote:

> On Mon, Sep 05, 2022 at 05:38:51PM +0300, Nikita Malakhov wrote:
> > Due to experiments with columnar data storage I've decided to revive this
> > thread - Table AM modifications to accept column projection lists
> > <
> https://www.postgresql.org/message-id/flat/cae-ml+9rmtnzkcntzpqf8o3b-ujhwgfbsoxpqa3wvuc8ybb...@mail.gmail.com
> >
> >
> > To remind:
> >
> > This patch introduces a set of changes to the table AM APIs, making them
> > accept a column projection list. That helps columnar table AMs, so that
> > they don't need to fetch all columns from disk, but only the ones
> > actually needed.
> >
> > The set of changes in this patch is not exhaustive -
> > there are many more opportunities that are discussed in the TODO section
> > below. Before digging deeper, we want to elicit early feedback on the
> > API changes and the column extraction logic.
> >
> > TableAM APIs that have been modified are:
> >
> > 1. Sequential scan APIs
> > 2. Index scan APIs
> > 3. API to lock and return a row
> > 4. API to fetch a single row
> >
> > We have seen performance benefits in Zedstore for many of the optimized
> > operations [0]. This patch is extracted from the larger patch shared in
> > [0].
>
> What parts of the original patch were left out ?  This seems to be the
> same size as the original.
>
> With some special build options like -DWRITE_READ_PARSE_PLAN_TREES, this
> currently fails with:
>
> WARNING:  outfuncs/readfuncs failed to produce equal parse tree
>
> There's poor code coverage in PopulateNeededColumnsForScan()
> IndexNext(), check_default_partition_contents() and nodeSeqscan.c.
> https://cirrus-ci.com/task/5516554904272896
>
> https://api.cirrus-ci.com/v1/artifact/task/5516554904272896/coverage/coverage/00-index.html
>
> Is it currently possible to hit those code paths in postgres ?  If not,
> you may need to invent a minimal columnar extension to allow excercising
> that.
>
> Note that the cirrusci link is on top of my branch which runs "extended"
> checks in cirrusci, but you can also run code coverage report locally
> with --enable-coverage.
>
> When you mail next, please run pgindent first (BTW there's a debian
> package in PGDG for pgindent).
>
> --
> Justin
>


-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

Remove dead macro exec_subplan_get_plan

[Zhang Mingli]

Hi,

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

Regards,
Zhang Mingli


vn-0001-exec_subplan_get_plan-is-not-used.patch
Description: Binary data

Re: Table AM modifications to accept column projection lists

[Justin Pryzby]

On Mon, Sep 05, 2022 at 05:38:51PM +0300, Nikita Malakhov wrote:
> Due to experiments with columnar data storage I've decided to revive this
> thread - Table AM modifications to accept column projection lists
> 
> 
> To remind:
> 
> This patch introduces a set of changes to the table AM APIs, making them
> accept a column projection list. That helps columnar table AMs, so that
> they don't need to fetch all columns from disk, but only the ones
> actually needed.
> 
> The set of changes in this patch is not exhaustive -
> there are many more opportunities that are discussed in the TODO section
> below. Before digging deeper, we want to elicit early feedback on the
> API changes and the column extraction logic.
> 
> TableAM APIs that have been modified are:
> 
> 1. Sequential scan APIs
> 2. Index scan APIs
> 3. API to lock and return a row
> 4. API to fetch a single row
> 
> We have seen performance benefits in Zedstore for many of the optimized
> operations [0]. This patch is extracted from the larger patch shared in
> [0].

What parts of the original patch were left out ?  This seems to be the
same size as the original.

With some special build options like -DWRITE_READ_PARSE_PLAN_TREES, this
currently fails with:

WARNING:  outfuncs/readfuncs failed to produce equal parse tree

There's poor code coverage in PopulateNeededColumnsForScan()
IndexNext(), check_default_partition_contents() and nodeSeqscan.c.
https://cirrus-ci.com/task/5516554904272896
https://api.cirrus-ci.com/v1/artifact/task/5516554904272896/coverage/coverage/00-index.html

Is it currently possible to hit those code paths in postgres ?  If not,
you may need to invent a minimal columnar extension to allow excercising
that.

Note that the cirrusci link is on top of my branch which runs "extended"
checks in cirrusci, but you can also run code coverage report locally
with --enable-coverage.

When you mail next, please run pgindent first (BTW there's a debian
package in PGDG for pgindent).

-- 
Justin

Re: ICU for global collation

[Marina Polyakova]

Hello!

IMO after adding ICU for global collations [1] the behaviour of createdb 
and CREATE DATABASE is a bit inconsistent when both locale and 
lc_collate (or locale and lc_ctype) options are used:


$ createdb mydb --locale C --lc-collate C --template template0
createdb: error: only one of --locale and --lc-collate can be specified
$ psql -c "create database mydb locale = 'C' lc_collate = 'C' template = 
'template0'" postgres

CREATE DATABASE

From the CREATE DATABASE documentation [2]:

locale
This is a shortcut for setting LC_COLLATE and LC_CTYPE at once. If you 
specify this, you cannot specify either of those parameters.


The patch diff_return_back_create_database_error.patch returns back the 
removed code for CREATE DATABASE so it behaves like createdb as 
before...


[1] 
https://github.com/postgres/postgres/commit/f2553d43060edb210b36c63187d52a632448e1d2

[2] https://www.postgresql.org/docs/devel/sql-createdatabase.html

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 6ff48bb18f3639ae45d9528b32df51a4aebc60c0..0a22cace11d9df6b5fc085bfd7b86319f4b13165 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -851,6 +851,12 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
 	 parser_errposition(pstate, defel->location)));
 	}
 
+	if (dlocale && (dcollate || dctype))
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ errdetail("LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE.")));
+
 	if (downer && downer->arg)
 		dbowner = defGetString(downer);
 	if (dtemplate && dtemplate->arg)

Re: Table AM modifications to accept column projection lists

[Nikita Malakhov]

Hi hackers!

Due to experiments with columnar data storage I've decided to revive this
thread -
Table AM modifications to accept column projection lists


To remind:

This patch introduces a set of changes to the table AM APIs, making them
accept a column projection list. That helps columnar table AMs, so that
they don't need to fetch all columns from disk, but only the ones
actually needed.

The set of changes in this patch is not exhaustive -
there are many more opportunities that are discussed in the TODO section
below. Before digging deeper, we want to elicit early feedback on the
API changes and the column extraction logic.

TableAM APIs that have been modified are:

1. Sequential scan APIs
2. Index scan APIs
3. API to lock and return a row
4. API to fetch a single row

We have seen performance benefits in Zedstore for many of the optimized
operations [0]. This patch is extracted from the larger patch shared in
[0].




-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/


v5_0001-tableam-accept-column-projection.patch
Description: Binary data

Re: Convert *GetDatum() and DatumGet*() macros to inline functions

[Aleksander Alekseev]

Hi Peter,

> Which compiler is that, by the way?

The warnings were reported by cfbot during the "clang_warning" step.
According to the logs:

```
using compiler=Debian clang version 11.0.1-2
```

Personally I use Clang 14 on MacOS and I don't get these warnings.

> I think to resolve that we could either
>
> 1. Not define PointerGetDatum() with a const argument, and just sprinkle
> in a few unconstify calls where necessary.
>
> 2. Maybe add a NonconstPointerGetDatum() for those few cases where
> pointer arguments are used for return values.
>
> 3. Go with your patch and just fix up the warnings about uninitialized
> variables.  But that seems the least principled to me.

IMO the 3rd option is the lesser evil. Initializing four bools/ints in
order to make Clang 11 happy doesn't strike me as such a big deal. At
least until somebody reports a bottleneck for this particular reason.
We can optimize the code when and if this will happen.

-- 
Best regards,
Aleksander Alekseev

Re: Reducing the chunk header sizes on all memory context types

[David Rowley]

On Fri, 2 Sept 2022 at 20:11, David Rowley  wrote:
>
> On Thu, 1 Sept 2022 at 12:46, Tom Lane  wrote:
> >
> > David Rowley  writes:
> > > Maybe we should just consider always making room for a sentinel for
> > > chunks that are on dedicated blocks. At most that's an extra 8 bytes
> > > in some allocation that's either over 1024 or 8192 (depending on
> > > maxBlockSize).
> >
> > Agreed, if we're not doing that already then we should.
>
> Here's a patch to that effect.

If there are no objections, then I plan to push that patch soon.

David

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[David Rowley]

On Mon, 5 Sept 2022 at 22:15, David Rowley  wrote:
> On Sat, 3 Sept 2022 at 00:37, Ranier Vilela  wrote:
> > 6. Avoid overhead when using unnecessary StringInfoData to convert Datum a 
> > to Text b.
>
> I've ripped out #4 and #6 for now. I think we should do #6 in master
> only, probably as part of a wider cleanup of StringInfo misusages.

I've attached a patch which does various other string operation cleanups.

* This changes cstring_to_text() to use cstring_to_text_with_len when
we're working with a StringInfo and can just access the .len field.
* Uses appendStringInfoString instead of appendStringInfo when there
is special formatting.
* Uses pstrdup(str) instead of psprintf("%s", str).  In many cases
this will save a bit of memory
* Uses appendPQExpBufferChar instead of appendPQExpBufferStr() when
appending a 1 byte string.
* Uses appendStringInfoChar() instead of appendStringInfo() when no
formatting and string is 1 byte.
* Uses appendStringInfoChar() instead of appendStringInfoString() when
string is 1 byte.
* Uses appendPQExpBuffer(b , ...) instead of appendPQExpBufferStr(b, "%s" ...)

I'm aware there are a few other places that we could use
cstring_to_text_with_len() instead of cstring_to_text(). For example,
using the return value of snprintf() to obtain the length. I just
didn't do that because we need to take care to check the return value
isn't -1.

My grep patterns didn't account for these function calls spanning
multiple lines, so I may have missed a few.

David
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index fb72bb6cfe..6161df2790 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -1325,7 +1325,7 @@ hstore_to_json_loose(PG_FUNCTION_ARGS)
}
appendStringInfoChar(, '}');
 
-   PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+   PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
 }
 
 PG_FUNCTION_INFO_V1(hstore_to_json);
@@ -1370,7 +1370,7 @@ hstore_to_json(PG_FUNCTION_ARGS)
}
appendStringInfoChar(, '}');
 
-   PG_RETURN_TEXT_P(cstring_to_text(dst.data));
+   PG_RETURN_TEXT_P(cstring_to_text_with_len(dst.data, dst.len));
 }
 
 PG_FUNCTION_INFO_V1(hstore_to_jsonb);
diff --git a/src/backend/access/brin/brin_minmax_multi.c 
b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..ea326a250b 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -3063,7 +3063,7 @@ brin_minmax_multi_summary_out(PG_FUNCTION_ARGS)
 
appendStringInfo(, "%s ... %s", a, b);
 
-   c = cstring_to_text(str.data);
+   c = cstring_to_text_with_len(str.data, str.len);
 
astate_values = accumArrayResult(astate_values,

 PointerGetDatum(c),
@@ -3095,15 +3095,9 @@ brin_minmax_multi_summary_out(PG_FUNCTION_ARGS)
{
Datum   a;
text   *b;
-   StringInfoData str;
-
-   initStringInfo();
 
a = FunctionCall1(, 
ranges_deserialized->values[idx++]);
-
-   appendStringInfoString(, DatumGetCString(a));
-
-   b = cstring_to_text(str.data);
+   b = cstring_to_text(DatumGetCString(a));
 
astate_values = accumArrayResult(astate_values,

 PointerGetDatum(b),
diff --git a/src/backend/replication/logical/tablesync.c 
b/src/backend/replication/logical/tablesync.c
index 91ba49a14b..78abbb8196 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1062,7 +1062,7 @@ copy_table(Relation rel)
appendStringInfoString(, 
quote_identifier(lrel.attnames[i]));
}
 
-   appendStringInfo(, ") TO STDOUT");
+   appendStringInfoString(, ") TO STDOUT");
}
else
{
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 081dfa2450..a2bdde0459 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -96,7 +96,7 @@ anytime_typmodout(bool istz, int32 typmod)
if (typmod >= 0)
return psprintf("(%d)%s", (int) typmod, tz);
else
-   return psprintf("%s", tz);
+   return pstrdup(tz);
 }
 
 
diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index 6594a9aac7..2b7b1b0c0f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1453,7 +1453,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
appendStringInfoChar(, ')');
 
if (idxrec->indnullsnotdistinct)
-   appendStringInfo(, " NULLS NOT DISTINCT");
+   appendStringInfoString(, " NULLS NOT DISTINCT");
 
/*
  

Re: Asynchronous execution support for Custom Scan

[Kazutaka Onishi]

Fujita-san,

I'm sorry for my error on your name...

>  IIUC, it uses the proposed
> APIs, but actually executes ctidscans *synchronously*, so it does not
> improve performance.  Right?

Exactly.
The actual CustomScan that supports asynchronous execution will
start processing in CustomScanAsyncRequest,
configure to detect completion via file descriptor in
CustomScanAsyncConfigureWait,
and receive the result in CustomScanAsyncNotify.

> So I'll review the proposed patches with it.
Thank you!

2022年9月5日(月) 15:27 Etsuro Fujita :
>
> On Fri, Sep 2, 2022 at 10:43 PM Kazutaka Onishi  wrote:
> > The asynchronous version "ctidscan" plugin is ready.
>
> Thanks for that!
>
> I looked at the extended version quickly.  IIUC, it uses the proposed
> APIs, but actually executes ctidscans *synchronously*, so it does not
> improve performance.  Right?
>
> Anyway, that version seems to be useful for testing that the proposed
> APIs works well.  So I'll review the proposed patches with it.  I'm
> not Fujii-san, though.  :-)
>
> Best regards,
> Etsuro Fujita

Re: Different compression methods for FPI

[Justin Pryzby]

On Mon, Sep 05, 2022 at 02:45:57PM +0200, Matthias van de Meent wrote:
> Hi,
> 
> I have a small question for those involved:

I suggest to "reply all"

> Context: I'm trying to get the smallest BKPIMAGE size possible
> regardless of CPU cost, which means I'm trying multiple compressions
> to get the smallest data possible with the options available. This
> means ignoring the wal_compression GUC in XLogCompressBackupBlock and
> brute-forcing the smallest compression available, potentially layering
> compression methods, and returning the bimg_info compression flags
> that will be stored in XLogCompressBackupBlock and used to decompress
> the block's data.

I think once you apply one compression method/algorithm, you shouldn't
expect other "layered" methods to be able to compress it at all.  I
think you'll get better compression by using a higher compression level
in zstd (or zlib) than with any combination of methods.  A patch for
configurable compression level was here:

https://postgr.es/m/2022031948.gj9...@telsasoft.com

> Is there a prescribed order of compression algorithms to apply when
> (de)compressing full page images in WAL, when the bimg_info has more
> than one BKPIMAGE_COMPRESS_*-flags set? That is, when I want to check
> the compression of the block image with both ZSTD and LZ4, which order
> is the ordering indicated by bimg_info = (COMPRESS_LZ4 |
> COMPRESS_ZSTD)?

There's currently a separate bit for each method, but it's not supported
to "stack" them (See also the "Save bits" patch, above).

This came up before when Greg asked about it.
https://www.postgresql.org/message-id/20210622031358.gf29...@telsasoft.com
https://www.postgresql.org/message-id/20220131222800.gy23...@telsasoft.com

-- 
Justin

Re: Convert *GetDatum() and DatumGet*() macros to inline functions

[Peter Eisentraut]

On 30.08.22 20:15, Aleksander Alekseev wrote:

Here is v3 with silenced compiler warnings.


Some more warnings were reported by cfbot, so here is v4. Apologies
for the noise.


Looking at these warnings you are fixing, I think there is a small 
problem we need to address.


I have defined PointerGetDatum() with a const argument:

PointerGetDatum(const void *X)

This is because in some places the thing that is being passed into that 
is itself defined as const, so this is the clean way to avoid warnings 
about dropping constness.


However, some support functions for gist and text search pass back 
return values via pointer arguments, like


DirectFunctionCall3(g_int_same,
entry->key,
PointerGetDatum(query),
PointerGetDatum());

The compiler you are using apparently thinks that passing  to a 
const pointer argument cannot change retval, which seems quite 
reasonable.  But that isn't actually what's happening here, so we're 
lying a bit.


(Which compiler is that, by the way?)

I think to resolve that we could either

1. Not define PointerGetDatum() with a const argument, and just sprinkle 
in a few unconstify calls where necessary.


2. Maybe add a NonconstPointerGetDatum() for those few cases where 
pointer arguments are used for return values.


3. Go with your patch and just fix up the warnings about uninitialized 
variables.  But that seems the least principled to me.

Re: Different compression methods for FPI

[Matthias van de Meent]

Hi,

I have a small question for those involved:

Context: I'm trying to get the smallest BKPIMAGE size possible
regardless of CPU cost, which means I'm trying multiple compressions
to get the smallest data possible with the options available. This
means ignoring the wal_compression GUC in XLogCompressBackupBlock and
brute-forcing the smallest compression available, potentially layering
compression methods, and returning the bimg_info compression flags
that will be stored in XLogCompressBackupBlock and used to decompress
the block's data.

Is there a prescribed order of compression algorithms to apply when
(de)compressing full page images in WAL, when the bimg_info has more
than one BKPIMAGE_COMPRESS_*-flags set? That is, when I want to check
the compression of the block image with both ZSTD and LZ4, which order
is the ordering indicated by bimg_info = (COMPRESS_LZ4 |
COMPRESS_ZSTD)?

Kind regards,

Matthias van de Meent

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[Ranier Vilela]

Em seg., 5 de set. de 2022 às 07:15, David Rowley 
escreveu:

> On Sat, 3 Sept 2022 at 00:37, Ranier Vilela  wrote:
> >> But +1 to fix this and other issues even if they would never crash.
>
> Yeah, I don't think any of this coding would lead to a crash, but it's
> pretty weird coding and we should fix it.
>
> > 1. Once that ranges->nranges is invariant, avoid the loop if
> ranges->nranges <= 0.
> > This matches the current behavior.
> >
> > 2. Once that ranges->nsorted is invariant, avoid the loop if
> ranges->nsorted <= 0.
> > This matches the current behavior.
> >
> > 3. Remove the invariant cxt from ranges->nsorted loop.
> >
> > 4. Avoid possible overflows when using int to store length strings.
> >
> > 5. Avoid possible out-of-bounds when ranges->nranges == 0.
> >
> > 6. Avoid overhead when using unnecessary StringInfoData to convert Datum
> a to Text b.
>
> I've ripped out #4 and #6 for now. I think we should do #6 in master
> only, probably as part of a wider cleanup of StringInfo misusages.
>
> I also spent some time trying to ensure I understand this code
> correctly.  I was unable to work out what the nsorted field was from
> just the comments. I needed to look at the code to figure it out, so I
> think the comments for that field need to be improved.  A few of the
> others were not that clear either.  I hope I've improved them in the
> attached.
>
> I was also a bit confused at various other comments. e.g:
>
> /*
> * Is the value greater than the maxval? If yes, we'll recurse to the
> * right side of range array.
> */
>
The second comment in the v3 patch, must be:
/*
* Is the value greater than the maxval? If yes, we'll recurse
* to the right side of the range array.
*/

I think this is copy-and-paste thinko with the word "minval".


>
> I don't see any sort of recursion going on here. All I see are
> skipping of values that are out of bounds of the lower bound of the
> lowest range, and above the upper bound of the highest range.
>
I think this kind recursion, because the loop is restarted
with:
start = (midpoint + 1);
continue;


>
> I propose to backpatch the attached into v14 tomorrow morning (about
> 12 hours from now).
>
> The only other weird coding I found was in brin_range_deserialize:
>
> for (i = 0; (i < nvalues) && (!typbyval); i++)
>
> I imagine most compilers would optimize that so that the typbyval
> check is done before the first loop and not done on every loop, but I
> don't think that coding practice helps the human readers out much. I
> left that one alone, for now.
>
Yeah, I prefer write:
if (!typbyval)
{
for (i = 0; (i < nvalues); i++)
}

regards,
Ranier Vilela

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Tomas Vondra]

On 9/5/22 12:12, Amit Kapila wrote:
> On Mon, Sep 5, 2022 at 12:14 PM Tomas Vondra
>  wrote:
>>
>> On 9/5/22 06:32, Amit Kapila wrote:
>>> On Sun, Sep 4, 2022 at 7:38 PM Tomas Vondra
>>>  wrote:

 On 9/4/22 14:24, Tomas Vondra wrote:
>
>> As per
>> my understanding, the problem I reported in the email [1] is the same
>> and we have seen this in BF failures as well. I posted a way to
>> reproduce it in that email. It seems this is possible if the decoding
>> gets XLOG_HEAP2_NEW_CID as the first record (belonging to a
>> subtransaction) after XLOG_RUNNING_XACTS.
>>
>
> Interesting. That's certainly true for WAL in the crashing case:
>
> rmgr: Standby len (rec/tot): 58/58, tx:  0, lsn:
> 0/01CDCD70, prev 0/01CDCD10, desc: RUNNING_XACTS nextXid 850
> latestCompletedXid 847 oldestRunningXid 848; 1 xacts: 848
> rmgr: Heap2   len (rec/tot): 60/60, tx:849, lsn:
> 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel 1663/16384/1249; tid
> 58/38; cmin: 1, cmax: 14, combo: 6
>

 I investigated using the pgdata from the crashed run (can provide, if
 you have rpi4 or some other aarch64 machine), and the reason is pretty
 simple - the restart_lsn for the slot is 0/1CDCD70, which is looong
 after the subxact assignment, so we add both xids as toplevel.

 That seems broken - if we skip the assignment like this, doesn't that
 break spill-to-disk and/or streaming? IIRC that's exactly why we had to
 start logging assignments immediately with wal_level=logical.

>>>
>>> We had started logging assignments immediately in commit 0bead9af48
>>> for streaming transactions in PG14. This issue exists prior to that. I
>>> have tried and reproduced it in PG13 but I think it will be there even
>>> before that. So, I am not sure if the spilling behavior is broken due
>>> to this. I think if we don't get assignment recording before
>>> processing changes during decoding commit then we could miss sending
>>> the changes which won't be the case here. Do you see any other
>>> problem?
>>>
>>
>> I can't, but that's hardly a proof of anything. You're right spilling to
>> disk may not be broken by this, though. I forgot it precedes assignments
>> being logged immediately, so it does not rely on that.
>>
 Or maybe we're not dealing with the restart_lsn properly, and we should
 have ignored those records. Both xacts started long before the restart
 LSN, so we're not seeing the whole xact anyway.

>>>
>>> Right, but is that problematic? The restart LSN will be used as a
>>> point to start reading the WAL and that helps in building a consistent
>>> snapshot. However, for decoding to send the commit, we use
>>> start_decoding_at point which will ensure that we send complete
>>> transactions.
>>>
>>
>> Which part would not be problematic? There's some sort of a bug, that's
>> for sure.
>>
> 
> It is possible that there is some other problem here that I am
> missing. But at this stage, I don't see anything wrong other than the
> assertion you have reported.
> 

I'm not sure I agree with that. I'm not convinced the assert is at
fault, it might easily be that it hints there's a logic bug somewhere.

>> I think it's mostly clear we won't output this transaction, because the
>> restart LSN is half-way through. We can either ignore it at commit time,
>> and then we have to make everything work in case we miss assignments (or
>> any other part of the transaction).
>>
> 
> Note, traditionally, we only form these assignments at commit time
> after deciding whether to skip such commits. So, ideally, there
> shouldn't be any fundamental problem with not making these
> associations before deciding whether we need to replay (send
> downstream) any particular transaction.
> 

Isn't that self-contradictory? Either we form these assignments at
commit time, or we support streaming (in which case it clearly can't
happen at commit time). AFAICS that's exactly why we started logging
(and processing) assignments immediately, no?

>> Or we can ignore stuff early, and not even process some of the changes.
>> For example in this case do we need to process the NEW_CID contents for
>> transaction 848? If we can skip that bit, the problem will disappear.
>>
>> But maybe this is futile and there are other similar issues ...
>>
 However, when processing the NEW_CID record:

 tx:849, lsn: 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel
 1663/16384/1249; tid 58/38; cmin: 1, cmax: 14, combo: 6

 we ultimately do this in SnapBuildProcessNewCid:

 #1  0x005566cccdb4 in ReorderBufferAddNewTupleCids (rb=0x559dd64218,
 xid=848, lsn=30264752, locator=..., tid=..., cmin=1, cmax=14,
 combocid=6) at reorderbuffer.c:3218
 #2  0x005566cd1f7c in SnapBuildProcessNewCid (builder=0x559dd6a248,
 xid=849, lsn=30264752, xlrec=0x559dd6e1e0) at snapbuild.c:818

Re: Mingw task for Cirrus CI

[Justin Pryzby]

On Sat, Sep 03, 2022 at 12:52:54AM +0300, Melih Mutlu wrote:
> Justin Pryzby , 19 Ağu 2022 Cum, 05:34 tarihinde şunu 
> yazdı:
> 
> > Inline notes about changes since the last version.
> >
> > On Thu, Jul 28, 2022 at 05:44:28PM -0500, Justin Pryzby wrote:
> > > I think the "only_if" should allow separately running one but not both
> > of the
> > > windows instances, like:
> > >
> > > +  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || 
> > > $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw64'
> > >
> > > I'm not sure, but maybe this task should only run "by request", and omit 
> > > the
> > > first condition:
> > >
> > > +  only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw64'
> >
> > The patch shouldn't say this during development, or else cfbot doesn't run 
> > it..
> > Oops.
>
> Actually, making MinGW task optional for now might make sense. Due to
> windows resource limits on Cirrus CI and slow builds on Windows, adding
> this task as non-optional may not be an efficient decision
> I think that continuing with this patch by changing MinGW to optional for
> now, instead of waiting for more resource on Cirrus or faster builds on
> Windows, could be better. I don't see any harm.

I agree that maybe it should be optional if merged to postgres.

But cfbot should run the Mingw task for this patch's own commitfest
entry.  But right now (because cfbot doesn't include the original commit
message/s), it doesn't get run :(

> +  only_if: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-include:[^\n]*mingw.* ||
> $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw.*'
> 
> Added this line to allow run only mingw task or run all tasks including
> mingw.
> 
> What do you all think about this change? Does it make sense?

You're allowing to write "ci-os-include: mingw" to "opt-in" to the task,
without opting-out of all the other tasks (and without enumerating all
the tasks by writing "ci-os-only: mingw,windows,macos,freebsd,linux".
That makes sense, and the logic looks right.  But that still has to be
commented during development to be run by cfbot.

Also, the first half was missing a closing quote.
https://cirrus-ci.com/build/5874178241855488

> > +  setup_additional_packages_script: |
> > > +REM C:\msys64\usr\bin\pacman.exe -S --noconfirm busybox
> >
> > This should include choco, too.
> 
> Added pacman.exe line. Do we really need choco here? I don't think mingw
> would require any package via choco.

I guess choco isn't needed.

> Also is ending pacman.exe line with busybox intentional? I just added that
> line with "..." at the end instead of any package name.

Yeah, the busybox part was unintentional.

> > >  for item in `find "$sourcetree" -name Makefile -print -o -name 
> > > GNUmakefile -print | grep -v "$sourcetree/doc/src/sgml/images/"`; do
> > > -filename=`expr "$item" : "$sourcetree\(.*\)"`
> > > -if test ! -f "${item}.in"; then
> > > -if cmp "$item" "$buildtree/$filename" >/dev/null 2>&1; then : ; 
> > > else
> > > -ln -fs "$item" "$buildtree/$filename" || exit 1
> > > -fi
> > > -fi
> > > +filename=${item#$sourcetree}
> > > +[ -e "$buildtree/$filename" ] && continue
> >
> > I fixed this to check for ".in" files as intended.
> >
> > It'd be a lot better if the image didn't take so long to start. :(
> 
> One question would be that should this patch include "prep_buildtree"? It
> doesn't seem to me like it's directly related to adding MinGW into CI but
> more like an improvement for builds on Windows.
> Maybe we can make it a seperate patch if it's necessary.

I don't know what direction that idea is going, but it makes working
with this patch a bit easier when configure is less slow.  Fine with me
to split it into a separate patch :)

> Sharing a new version of the patch. It also moves the above line so that it
> will apply to mingw task too. Otherwise mingw task was failing.

I saw that but hadn't tracked it down yet.  Do you know if the tar
failures were from a TAP test added since you first posted the mingw
patch, or ??

Also: your original patch said --host=x86_64-w64-mingw32, but the task
is called MinGW64.  The most recent patches don't use --host at all, and
were building for a 32 bit environment, even though the OS image says
MSYSTEM=UCRT64.

Also: right now src/test and src/interfaces/*/test aren't being built
during the build phase, which means that they're 1) not compiled in
parallel; and 2) not cached.  This isn't specific to MinGW.  Other than
compiling those dirs specifically, one option is to put
"always: upload_caches: ccache" after "test_world_script" (in that case,
if the CI instance is rescheduled during tests, the compilation won't be
pushed to cache).  Actually, it seems better to compile stuff during
"build" or else any compilation warnings should up in the middle of
"check-world.."

Also: I'm having second thoughts about the loop around ./configure.  It
could happen that a cached configure would succeed, 

Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))

[Justin Pryzby]

On Tue, Jul 26, 2022 at 03:38:53PM -0700, David G. Johnston wrote:
> On Mon, Jan 24, 2022 at 9:54 AM Justin Pryzby  wrote:

> > > Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate.  If 
> > > specifying
> > > "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I 
> > > suppose
> > > this patch series could do as suggested and enable buffers by default 
> > > only when
> > > ANALYZE is specified.  Then postgres_fdw is not affected, and the
> > > explain_regress GUC is optional: instead, we'd need to specify BUFFERS 
> > > OFF in
> > > ~100 regression tests which use EXPLAIN ANALYZE.
> 
> I'm not following the transition from the prior sentences about COSTS to
> this one regarding BUFFERS.

In this patch, SET explain_regress=on changes to defaults to
explain(COSTS OFF), so postgres_fdw now explicitly uses COSTS ON.  The
patch also changes default to (BUFFERS OFF), which is what allows
enabling buffers by default.  I think I was just saying that the
enabling buffers by default depends on somehow disabling it in
regression tests (preferably not by adding 100 instances of "BUFFERS
OFF").

> The following change in the machine commit seems out-of-place; are we
> fixing a bug here?
> 
> explain.c:
>/* Show buffer usage in planning */
> - if (bufusage)
> + if (bufusage && es->buffers)

I think that was an extraneous change, maybe from a conflict while
re-ordering the patches.  Removed it and rebased.  Thanks for looking.

-- 
Justin
>From 0bf5296d9e71e136509557fa62c6479965c753e8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress

This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
 contrib/postgres_fdw/postgres_fdw.c |  2 +-
 src/backend/commands/explain.c  | 13 +++--
 src/backend/utils/misc/guc.c| 13 +
 src/include/commands/explain.h  |  2 ++
 src/test/regress/expected/explain.out   |  3 +++
 src/test/regress/expected/inherit.out   |  2 +-
 src/test/regress/expected/stats_ext.out |  2 +-
 src/test/regress/pg_regress.c   |  3 ++-
 src/test/regress/sql/explain.sql|  4 
 src/test/regress/sql/inherit.sql|  2 +-
 src/test/regress/sql/stats_ext.sql  |  2 +-
 11 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 16320170cee..68f3e97e13b 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3131,7 +3131,7 @@ estimate_path_cost_size(PlannerInfo *root,
 		 * values, so don't request params_list.
 		 */
 		initStringInfo();
-		appendStringInfoString(, "EXPLAIN ");
+		appendStringInfoString(, "EXPLAIN (COSTS)");
 		deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
 remote_conds, pathkeys,
 fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 053d2ca5ae4..f3ce3593f36 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
 static void ExplainYAMLLineStarting(ExplainState *es);
 static void escape_yaml(StringInfo buf, const char *str);
 
+bool explain_regress = false; /* GUC */
 
 
 /*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	ListCell   *lc;
 	bool		timing_set = false;
 	bool		summary_set = false;
+	bool		costs_set = false;
 
 	/* Parse options list. */
 	foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 		else if (strcmp(opt->defname, "verbose") == 0)
 			es->verbose = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "costs") == 0)
+		{
+			/* Need to keep track if it was explicitly set to ON */
+			costs_set = true;
 			es->costs = defGetBoolean(opt);
+		}
 		else if (strcmp(opt->defname, "buffers") == 0)
 			es->buffers = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	 parser_errposition(pstate, opt->location)));
 	}
 
+	/* if the costs option was not set explicitly, set default value */
+	es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
 	if (es->wal && !es->analyze)
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("EXPLAIN option WAL requires ANALYZE")));
 
 	/* if the timing was not set explicitly, set default value */
-	es->timing = (timing_set) ? es->timing : es->analyze;
+	es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
 
 	/* check that timing is used with EXPLAIN ANALYZE */
 	if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
  errmsg("EXPLAIN option TIMING 

Re: [POC] Allow flattening of subquery with a link to upper query

[Andrey Lepikhov]

On 9/5/22 12:22, Richard Guo wrote:


On Fri, Sep 2, 2022 at 7:09 PM Andrey Lepikhov 
Yeah, it's not easy-to-solve problem. If I correctly understand the

code, to fix this problem we must implement the same logic, as
pull_up_subqueries (lowest_outer_join/safe_upper_varnos). 


Yeah, I think we'd have to consider the restrictions from lateral
references to guarantee correctness when we pull up subqueries. We need
to avoid the situation where quals need to be postponed past outer join.

However, even if we have taken care of that, there may be other issues
with flattening direct-correlated ANY SubLink. The constraints imposed
by LATERAL references may make it impossible for us to find any legal
join orders, as discussed in [1].

[1] 
https://www.postgresql.org/message-id/CAMbWs49cvkF9akbomz_fCCKS=D5TY=4kgheqcfhpzcxs1gv...@mail.gmail.com 


The problem you mentioned under this link is about ineffective query 
plan - as I understand it.
This is a problem, especially if we would think about more complex 
pull-ups of subqueries - with aggregate functions in the target list.
I think about that problem as about next step - we already have an 
example - machinery of alternative plans. This problem may be solved in 
this way, or by a GUC, as usual.


--
Regards
Andrey Lepikhov
Postgres Professional

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Tomas Vondra]

On 9/5/22 08:35, Amit Kapila wrote:
> On Sun, Sep 4, 2022 at 11:10 PM Tomas Vondra
>  wrote:
>>
>> On 9/4/22 16:08, Tomas Vondra wrote:
>>> ...
>>>
>>> so in fact we *know* 849 is a subxact of 848, but we don't call
>>> ReorderBufferAssignChild in this case. In fact we can't even do the
>>> assignment easily in this case, because we create the subxact first, so
>>> that the crash happens right when we attempt to create the toplevel one,
>>> and we never even get a chance to do the assignment:
>>>
>>> 1) process the NEW_CID record, logged for 849 (subxact)
>>> 2) process CIDs in the WAL record, which has topleve_xid 848
>>>
>>>
>>> So IMHO we need to figure out what to do for WAL records that create
>>> both the toplevel and subxact - either we need to skip them, or rethink
>>> how we create the ReorderBufferTXN structs.
>>>
>>
>> This fixes the crash for me, by adding a ReorderBufferAssignChild call
>> to SnapBuildProcessNewCid, and tweaking ReorderBufferAssignChild to
>> ensure we don't try to create the top xact before updating the subxact
>> and removing it from the toplevel_by_lsn list.
>>
>> Essentially, what's happening is this:
>>
>> 1) We read the NEW_CID record, which is logged with XID 849, i.e. the
>> subxact. But we don't know it's a subxact, so we create it as a
>> top-level xact with the LSN.
>>
>> 2) We start processing contents of the NEW_CID, which however has info
>> that 849 is subxact of 848, calls ReorderBufferAddNewTupleCids which
>> promptly does ReorderBufferTXNByXid() with the top-level XID, which
>> creates it with the same LSN, and crashes because of the assert.
>>
>> I'm not sure what's the right/proper way to fix this ...
>>
>> The problem is ReorderBufferAssignChild was coded in a way that did not
>> expect the subxact to be created first (as a top-level xact).
>>
> 
> I think there was a previously hard-coded way to detect that and we
> have removed it in commit
> (https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e3ff789acfb2754cd7b5e87f6f4463fd08e35996).
> I think it is possible that subtransaction gets logged without
> previous top-level txn record as shown in the commit shared.
> 

Well, yes and no.

This wouldn't detect the issue, because the assert happens in the first
ReorderBufferTXNByXid(), so it's still crash (in assert-enabled build,
at least).

Maybe removing the assumption was the wrong thing, and we should have
changed the code so that we don't violate it? That's kinda what my "fix"
does, in a way.

regards

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

Re: Column Filtering in Logical Replication

[Amit Kapila]

On Mon, Sep 5, 2022 at 3:46 PM Peter Smith  wrote:
>
>
> PSA v7.
>

For example, if additional columns are added to the table, then
+(after a REFRESH PUBLICATION) if there was a column list
+only those named columns will continue to be replicated.

This looks a bit unclear to me w.r.t the refresh publication step. Why
exactly you have used refresh publication in the above para? It is
used to add new tables if any added to the publication, so not clear
to me how it helps in this case. If that is not required then we can
change it to: "For example, if additional columns are added to the
table then only those named columns mentioned in the column list will
continue to be replicated."

-- 
With Regards,
Amit Kapila.

Re: Column Filtering in Logical Replication

[Peter Smith]

On Mon, Sep 5, 2022 at 1:42 PM shiy.f...@fujitsu.com
 wrote:
>
> On Mon, Sep 5, 2022 8:28 AM Peter Smith  wrote:
> >
> > I have rebased the remaining patch (v6-0001 is the same as v5-0002)
> >
>
> Thanks for updating the patch. Here are some comments.
>
> 1.
> + the  will be successful but later
> + the WalSender on the publisher, or the subscriber may throw an error. In
> + this scenario, the user needs to recreate the subscription after 
> adjusting
>
> Should "WalSender" be changed to "walsender"? I saw "walsender" is used in 
> other
> places in the documentation.

Modified.

>
> 2.
> +test_pub=# CREATE TABLE t1(id int, a text, b text, c text, d text, e text, 
> PRIMARY KEY(id));
> +CREATE TABLE
> +test_pub=#
>
> +test_pub=# CREATE PUBLICATION p1 FOR TABLE t1 (id, b, a, d);
> +CREATE PUBLICATION
> +test_pub=#
>
> I think the redundant "test_pub=#" can be removed.
>

Modified.

>
> Besides, I tested the examples in the patch, there's no problem.
>

Thanks for the review comments, and testing.

I made both fixes as suggested.

PSA v7.

--
Kind Regards,
Peter Smith.
Fujitsu Australia


v7-0001-Column-List-new-pgdocs-section.patch
Description: Binary data

Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

[David Rowley]

On Sat, 3 Sept 2022 at 00:37, Ranier Vilela  wrote:
>> But +1 to fix this and other issues even if they would never crash.

Yeah, I don't think any of this coding would lead to a crash, but it's
pretty weird coding and we should fix it.

> 1. Once that ranges->nranges is invariant, avoid the loop if ranges->nranges 
> <= 0.
> This matches the current behavior.
>
> 2. Once that ranges->nsorted is invariant, avoid the loop if ranges->nsorted 
> <= 0.
> This matches the current behavior.
>
> 3. Remove the invariant cxt from ranges->nsorted loop.
>
> 4. Avoid possible overflows when using int to store length strings.
>
> 5. Avoid possible out-of-bounds when ranges->nranges == 0.
>
> 6. Avoid overhead when using unnecessary StringInfoData to convert Datum a to 
> Text b.

I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.

I also spent some time trying to ensure I understand this code
correctly.  I was unable to work out what the nsorted field was from
just the comments. I needed to look at the code to figure it out, so I
think the comments for that field need to be improved.  A few of the
others were not that clear either.  I hope I've improved them in the
attached.

I was also a bit confused at various other comments. e.g:

/*
* Is the value greater than the maxval? If yes, we'll recurse to the
* right side of range array.
*/

I don't see any sort of recursion going on here. All I see are
skipping of values that are out of bounds of the lower bound of the
lowest range, and above the upper bound of the highest range.

I propose to backpatch the attached into v14 tomorrow morning (about
12 hours from now).

The only other weird coding I found was in brin_range_deserialize:

for (i = 0; (i < nvalues) && (!typbyval); i++)

I imagine most compilers would optimize that so that the typbyval
check is done before the first loop and not done on every loop, but I
don't think that coding practice helps the human readers out much. I
left that one alone, for now.

David
diff --git a/src/backend/access/brin/brin_minmax_multi.c 
b/src/backend/access/brin/brin_minmax_multi.c
index a581659fe2..99ef97206a 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -149,13 +149,16 @@ typedef struct MinMaxMultiOptions
  * single-point ranges. That is, we have (2*nranges + nvalues) boundary
  * values in the array.
  *
- * +-+---+
- * | ranges (sorted pairs of values) | sorted values (single points) |
- * +-+---+
+ * +-+--+
+ * | ranges (2 * nranges of) | single point values (nvalues of) |
+ * +-+--+
  *
  * This allows us to quickly add new values, and store outliers without
  * making the other ranges very wide.
  *
+ * 'nsorted' denotes how many of 'nvalues' in the values[] array are sorted.
+ * When nsorted == nvalues, all single point values are sorted.
+ *
  * We never store more than maxvalues values (as set by values_per_range
  * reloption). If needed we merge some of the ranges.
  *
@@ -173,10 +176,10 @@ typedef struct Ranges
FmgrInfo   *cmp;
 
/* (2*nranges + nvalues) <= maxvalues */
-   int nranges;/* number of ranges in 
the array (stored) */
-   int nsorted;/* number of sorted 
values (ranges + points) */
-   int nvalues;/* number of values in 
the data array (all) */
-   int maxvalues;  /* maximum number of 
values (reloption) */
+   int nranges;/* number of ranges in 
the values[] array */
+   int nsorted;/* number of nvalues 
which are sorted */
+   int nvalues;/* number of point 
values in values[] */
+   int maxvalues;  /* number of elements 
in the values[] array */
 
/*
 * We simply add the values into a large buffer, without any expensive
@@ -318,102 +321,99 @@ AssertCheckRanges(Ranges *ranges, FmgrInfo *cmpFn, Oid 
colloid)
 * Check that none of the values are not covered by ranges (both sorted
 * and unsorted)
 */
-   for (i = 0; i < ranges->nvalues; i++)
+   if (ranges->nranges > 0)
{
-   Datum   compar;
-   int start,
-   end;
-   Datum   minvalue,
-   maxvalue;
-
-   Datum   value = ranges->values[2 * ranges->nranges + i];
-
-   if (ranges->nranges == 0)
-   break;
-
-   

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Amit Kapila]

On Mon, Sep 5, 2022 at 12:14 PM Tomas Vondra
 wrote:
>
> On 9/5/22 06:32, Amit Kapila wrote:
> > On Sun, Sep 4, 2022 at 7:38 PM Tomas Vondra
> >  wrote:
> >>
> >> On 9/4/22 14:24, Tomas Vondra wrote:
> >>>
>  As per
>  my understanding, the problem I reported in the email [1] is the same
>  and we have seen this in BF failures as well. I posted a way to
>  reproduce it in that email. It seems this is possible if the decoding
>  gets XLOG_HEAP2_NEW_CID as the first record (belonging to a
>  subtransaction) after XLOG_RUNNING_XACTS.
> 
> >>>
> >>> Interesting. That's certainly true for WAL in the crashing case:
> >>>
> >>> rmgr: Standby len (rec/tot): 58/58, tx:  0, lsn:
> >>> 0/01CDCD70, prev 0/01CDCD10, desc: RUNNING_XACTS nextXid 850
> >>> latestCompletedXid 847 oldestRunningXid 848; 1 xacts: 848
> >>> rmgr: Heap2   len (rec/tot): 60/60, tx:849, lsn:
> >>> 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel 1663/16384/1249; tid
> >>> 58/38; cmin: 1, cmax: 14, combo: 6
> >>>
> >>
> >> I investigated using the pgdata from the crashed run (can provide, if
> >> you have rpi4 or some other aarch64 machine), and the reason is pretty
> >> simple - the restart_lsn for the slot is 0/1CDCD70, which is looong
> >> after the subxact assignment, so we add both xids as toplevel.
> >>
> >> That seems broken - if we skip the assignment like this, doesn't that
> >> break spill-to-disk and/or streaming? IIRC that's exactly why we had to
> >> start logging assignments immediately with wal_level=logical.
> >>
> >
> > We had started logging assignments immediately in commit 0bead9af48
> > for streaming transactions in PG14. This issue exists prior to that. I
> > have tried and reproduced it in PG13 but I think it will be there even
> > before that. So, I am not sure if the spilling behavior is broken due
> > to this. I think if we don't get assignment recording before
> > processing changes during decoding commit then we could miss sending
> > the changes which won't be the case here. Do you see any other
> > problem?
> >
>
> I can't, but that's hardly a proof of anything. You're right spilling to
> disk may not be broken by this, though. I forgot it precedes assignments
> being logged immediately, so it does not rely on that.
>
> >> Or maybe we're not dealing with the restart_lsn properly, and we should
> >> have ignored those records. Both xacts started long before the restart
> >> LSN, so we're not seeing the whole xact anyway.
> >>
> >
> > Right, but is that problematic? The restart LSN will be used as a
> > point to start reading the WAL and that helps in building a consistent
> > snapshot. However, for decoding to send the commit, we use
> > start_decoding_at point which will ensure that we send complete
> > transactions.
> >
>
> Which part would not be problematic? There's some sort of a bug, that's
> for sure.
>

It is possible that there is some other problem here that I am
missing. But at this stage, I don't see anything wrong other than the
assertion you have reported.

> I think it's mostly clear we won't output this transaction, because the
> restart LSN is half-way through. We can either ignore it at commit time,
> and then we have to make everything work in case we miss assignments (or
> any other part of the transaction).
>

Note, traditionally, we only form these assignments at commit time
after deciding whether to skip such commits. So, ideally, there
shouldn't be any fundamental problem with not making these
associations before deciding whether we need to replay (send
downstream) any particular transaction.

> Or we can ignore stuff early, and not even process some of the changes.
> For example in this case do we need to process the NEW_CID contents for
> transaction 848? If we can skip that bit, the problem will disappear.
>
> But maybe this is futile and there are other similar issues ...
>
> >> However, when processing the NEW_CID record:
> >>
> >> tx:849, lsn: 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel
> >> 1663/16384/1249; tid 58/38; cmin: 1, cmax: 14, combo: 6
> >>
> >> we ultimately do this in SnapBuildProcessNewCid:
> >>
> >> #1  0x005566cccdb4 in ReorderBufferAddNewTupleCids (rb=0x559dd64218,
> >> xid=848, lsn=30264752, locator=..., tid=..., cmin=1, cmax=14,
> >> combocid=6) at reorderbuffer.c:3218
> >> #2  0x005566cd1f7c in SnapBuildProcessNewCid (builder=0x559dd6a248,
> >> xid=849, lsn=30264752, xlrec=0x559dd6e1e0) at snapbuild.c:818
> >>
> >> so in fact we *know* 849 is a subxact of 848, but we don't call
> >> ReorderBufferAssignChild in this case. In fact we can't even do the
> >> assignment easily in this case, because we create the subxact first, so
> >> that the crash happens right when we attempt to create the toplevel one,
> >> and we never even get a chance to do the assignment:
> >>
> >> 1) process the NEW_CID record, logged for 849 (subxact)
> >> 2) process CIDs in the WAL record, which has topleve_xid 

Re: freeing LDAPMessage in CheckLDAPAuth

[Zhihong Yu]

On Sun, Sep 4, 2022 at 10:37 PM Michael Paquier  wrote:

> On Sun, Sep 04, 2022 at 06:52:37AM -0700, Zhihong Yu wrote:
> > Please take a look at patch v3.
>
> Fine as far as it goes.  I would have put the initialization of
> search_message closer to ldap_search_s() for consistency with libpq.
> That's what we do with ldap_search_st().
> --
> Michael
>
Hi,
Here is patch v4.


v4-ldap-msg-free.patch
Description: Binary data

Re: Handle infinite recursion in logical replication setup

[Amit Kapila]

On Mon, Sep 5, 2022 at 9:47 AM Peter Smith  wrote:
>
> Here are my review comments for v45-0001:
>
> ==
>
> 1. doc/src/sgml/logical-replication.sgml
>
>   
>To find which tables might potentially include non-local origins (due to
>other subscriptions created on the publisher) try this SQL query:
> 
> SELECT DISTINCT N.nspname AS schemaname, C.relname AS tablename
> FROM pg_publication P,
>  LATERAL pg_get_publication_tables(P.pubname) GPT
>  LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
>  pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
> WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND
>   P.pubname IN (pub-names);
> 
>
> 1a.
> To use "" with the <> then simply put meta characters in the SGML.
> e.g.
> pub-names
>
> ~
>
> 1b.
> The patch forgot to add the SQL "#" instruction as suggested by my
> previous comment (see [1] #3b)
>
> ~~~
>
> 2.
>
>  
>   Preventing Data Inconsistencies for copy_data, origin=NONE
>
> The title is OK, but I think this should all be a  sub-section
> of "31.2 Subscription"
>
> ==
>

It is recommended to create the subscription
+   using enabled=false, so that if the origin WARNING occurs
+   no copy has happened yet. Otherwise some corrective steps might be needed to
+   remove any unwanted data that got copied.

I am not completely sure of this part of the docs as this can add
additional steps for users while working on subscriptions even when
the same is not required. I suggest for now we can remove this part.
Later based on some feedback on this feature, we can extend the docs
if required.

Also, instead of having it as a separate section, let's keep this as
part of create_subscription.sgml

*
+ errhint("Verify that initial data copied from the publisher tables
did not come from other origins. Some corrective action may be
necessary."));

The second sentence in this message doesn't seem to be required.


-- 
With Regards,
Amit Kapila.

Re: Missing CFI in iterate_word_similarity()

[Daniel Gustafsson]

> On 2 Sep 2022, at 15:22, Daniel Gustafsson  wrote:
> 
>> On 2 Sep 2022, at 15:16, Tom Lane  wrote:
> 
>> What's annoying me about the one-liner fix is that it makes it
>> look like CFI is part of the "Get index" action.
> 
> Thats a good point, I'll split the code up to make it clearer.

Done that way and pushed, thanks!

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

Re: pg15b3: recovery fails with wal prefetch enabled

[Thomas Munro]

On Mon, Sep 5, 2022 at 5:34 PM Kyotaro Horiguchi
 wrote:
> At Mon, 05 Sep 2022 14:15:27 +0900 (JST), Kyotaro Horiguchi 
>  wrote in
> me> +1 for showing any message for the failure, but I think we shouldn't
> me> hide an existing message if any.
>
> At Mon, 5 Sep 2022 16:54:07 +1200, Thomas Munro  
> wrote in
> > On reflection, it'd be better not to clobber any pre-existing error
> > there, but report one only if there isn't one already queued.  I've
> > done that in this version, which I'm planning to do a bit more testing
> > on and commit soonish if there are no comments/objections, especially
> > for that part.
>
> It looks fine in this regard.  I still think that the message looks
> somewhat internal.

Thanks for looking!

> me> And the error messages around are
> me> just telling that " at RecPtr". So I think
> me> "missing contrecord at RecPtr" is sufficient here.

Ok, I've updated it like that.

> > I'll have to check whether a doc change is necessary somewhere to
> > advertise that maintenance_io_concurrency=0 turns off prefetching, but
> > IIRC that's kinda already implied.
> >
> > I've tested quite a lot of scenarios including make check-world with
> > maintenance_io_concurrency = 0, 1, 10, 1000, and ALTER SYSTEM for all
> > relevant GUCs on a standby running large pgbench to check expected
> > effect on pg_stat_recovery_prefetch view and generate system calls.
>
> +   if (likely(record = prefetcher->reader->record))
>
> Isn't this confusing a bit?
>
>
> +   if (likely(record = prefetcher->reader->record))
> +   {
> +   XLogRecPtr  replayed_up_to = record->next_lsn;
> +
> +   XLogReleasePreviousRecord(prefetcher->reader);
> +
>
> The likely condition is the prerequisite for
> XLogReleasePreviousRecord.  But is is a little hard to read the
> condition as "in case no previous record exists".  Since there is one
> in most cases, can't call XLogReleasePreviousRecord() unconditionally
> then the function returns true when the previous record exists and
> false if not?

We also need the LSN that is past that record.
XLogReleasePreviousRecord() could return it (or we could use
reader->EndRecPtr I suppose).  Thoughts on this version?
From efa095529b3615f30b710a4b42bd0904b187af53 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 5 Sep 2022 12:07:24 +1200
Subject: [PATCH v3] Fix recovery_prefetch with low maintenance_io_concurrency.

The LSN-based logic for knowing when IOs complete ran operations in the
wrong order.  We should process completed IOs first before trying to
start more, so that it is always possible to decode one more record when
the decoded record queue is empty, even if maintenance_io_concurrency is
set so low that a single earlier WAL record might have saturated the IO
queue.

That thinko was hidden because the effect of maintenance_io_concurrency
was arbitrarily clamped to be at least 2.  Fix the ordering, and also
remove that clamp.  We need a special case for 0, which is now treated
the same as recovery_prefetch=off, but otherwise the number is used
directly.  This allows for testing with 1, which would have made the
problem obvious in simple test scenarios.

Also add an explicit error message for missing contrecords.  It's needed
for correctness with the resulting slightly more aggressive prefetching,
to avoid losing track of that state, but it's also a bit strange that we
didn't have an error message already.

Back-patch to 15.

Reported-by: Justin Pryzby 
Reviewed-by: Kyotaro Horiguchi 
Discussion: https://postgr.es/m/20220831140128.GS31833%40telsasoft.com
---
 src/backend/access/transam/xlogprefetcher.c | 54 ++---
 src/backend/access/transam/xlogreader.c | 22 +++--
 src/include/access/xlogreader.h |  2 +-
 3 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index 9aa56411d5..91bfba30ec 100644
--- a/src/backend/access/transam/xlogprefetcher.c
+++ b/src/backend/access/transam/xlogprefetcher.c
@@ -72,7 +72,9 @@
 int			recovery_prefetch = RECOVERY_PREFETCH_TRY;
 
 #ifdef USE_PREFETCH
-#define RecoveryPrefetchEnabled() (recovery_prefetch != RECOVERY_PREFETCH_OFF)
+#define RecoveryPrefetchEnabled() \
+		(recovery_prefetch != RECOVERY_PREFETCH_OFF && \
+		 maintenance_io_concurrency > 0)
 #else
 #define RecoveryPrefetchEnabled() false
 #endif
@@ -985,6 +987,7 @@ XLogRecord *
 XLogPrefetcherReadRecord(XLogPrefetcher *prefetcher, char **errmsg)
 {
 	DecodedXLogRecord *record;
+	XLogRecPtr	replayed_up_to;
 
 	/*
 	 * See if it's time to reset the prefetching machinery, because a relevant
@@ -1000,7 +1003,8 @@ XLogPrefetcherReadRecord(XLogPrefetcher *prefetcher, char **errmsg)
 
 		if (RecoveryPrefetchEnabled())
 		{
-			max_inflight = Max(maintenance_io_concurrency, 2);
+			Assert(maintenance_io_concurrency > 0);
+			max_inflight = maintenance_io_concurrency;
 			max_distance = max_inflight * 

Re: Patch to address creation of PgStat* contexts with null parent context

[Kyotaro Horiguchi]

At Mon, 5 Sep 2022 08:52:44 +0200, "Drouvot, Bertrand"  
wrote in 
> Could using TopMemoryContext like in the attach be an option? (aka
> changing CacheMemoryContext by TopMemoryContext in the 3 places of
> interest): that would ensure the 3 pgStat* contexts to have a non NULL
> parent context.

Of course it works. The difference from what I last proposed is
whether we postpone creating the memory contexts until the first call
to pgstat_get_entry_ref().  The rationale of creating them at
pgstat_attach_shmem is that anyway once pgstat_attach_shmem is called,
the process fainally creates the contexts at the end of the process,
and (I think) it's simpler that we don't do if() check at every
pgstat_get_entry_ref() call.

I forgot about pgStatPendingContext, but it is sensible that we treat
it the same way to the other two.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Fix typo function circle_same (src/backend/utils/adt/geo_ops.c)

[Daniel Gustafsson]

> On 4 Sep 2022, at 14:39, Ranier Vilela  wrote:

> But with Windows 10 build, I got this diff result:
> 
> diff -w -U3 
> C:/dll/postgres_dev/postgres_master/src/test/regress/expected/geometry.out 
> C:/dll/postgres_dev/postgres_master/src/test/regress/results/geometry.out
> --- 
> C:/dll/postgres_dev/postgres_master/src/test/regress/expected/geometry.out 
> 2022-09-01 08:05:03.685931000 -0300
> +++ C:/dll/postgres_dev/postgres_master/src/test/regress/results/geometry.out 
> 2022-09-04 09:27:47.133617800 -0300
> @@ -4380,9 +4380,8 @@
>   <(100,200),10> | <(100,200),10>
>   <(100,1),115>  | <(100,1),115>
>   <(3,5),0>  | <(3,5),0>
> - <(3,5),NaN>| <(3,5),0>
>   <(3,5),NaN>| <(3,5),NaN>
> -(9 rows)
> +(8 rows)
>  
>  -- Overlap with circle
>  SELECT c1.f1, c2.f1 FROM CIRCLE_TBL c1, CIRCLE_TBL c2 WHERE c1.f1 && c2.f1;
> 
> Not sure why.

That's not just on Windows, and it makes total sense as a radius of NaN isn't
the same as a radius of zero.

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

Re: [BUG] Storage declaration in ECPG

[Kyotaro Horiguchi]

At Sun, 04 Sep 2022 13:49:53 +0300, Andrey Sokolov  
wrote in 
> Hi,
> 
> The ECPG preprocessor converts the code
> "static VARCHAR str1[10], str2[20], str3[30];"
> into
> "static  struct varchar_1  { int len; char arr[ 10 ]; }  str1 ;
>  struct varchar_2  { int len; char arr[ 20 ]; }  str2 ;
>  struct varchar_3  { int len; char arr[ 30 ]; }  str3 ;".
> Storage declaration applies only to the first structure.

Good catch!

> The patch in the attachment fixes the bug. Storage declaration will be
> repeated before each structure.
> The patch is on github too:
> https://github.com/andr-sokolov/postgresql/commit/c8f8fc7a211938569e7d46c91a428d8cb25b6f9c

And the code looks good to me.

About the test, don't we need the test for non-varchar/bytea static
variables like "static int inta, intb, intc;"?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [POC] Allow flattening of subquery with a link to upper query

[Richard Guo]

On Fri, Sep 2, 2022 at 7:09 PM Andrey Lepikhov 
wrote:

> On 9/1/22 17:24, Richard Guo wrote:
> > On Wed, Aug 31, 2022 at 2:35 PM Andrey Lepikhov
> > mailto:a.lepik...@postgrespro.ru>> wrote:
> > Before flattening procedure we just look through the quals of
> subquery,
> > pull to the upper level OpExpr's containing variables from the upper
> > relation and replace their positions in the quals with true
> expression.
> > Further, the flattening machinery works as usual.
> >
> > Hmm, I'm not sure this patch works correctly in all cases. It seems to
> > me this patch pulls up the subquery without checking the constraints
> > imposed by lateral references. If its quals contain any lateral
> > references to rels outside a higher outer join, we would need to
> > postpone quals from below an outer join to above it, which is probably
> > incorrect.



> Yeah, it's not easy-to-solve problem. If I correctly understand the
> code, to fix this problem we must implement the same logic, as
> pull_up_subqueries (lowest_outer_join/safe_upper_varnos).


Yeah, I think we'd have to consider the restrictions from lateral
references to guarantee correctness when we pull up subqueries. We need
to avoid the situation where quals need to be postponed past outer join.

However, even if we have taken care of that, there may be other issues
with flattening direct-correlated ANY SubLink. The constraints imposed
by LATERAL references may make it impossible for us to find any legal
join orders, as discussed in [1].

[1]
https://www.postgresql.org/message-id/CAMbWs49cvkF9akbomz_fCCKS=D5TY=4kgheqcfhpzcxs1gv...@mail.gmail.com

Thanks
Richard

Re: Clarify restriction on partitioned tables primary key / unique indexes

[David Rowley]

On Fri, 2 Sept 2022 at 22:06, David Rowley  wrote:
> Thanks.  I ended up adjusting it to:
>
> "To create a unique or primary key constraint on a partitioned table,"

and pushed.

Thanks for having a look at this Erik.

David

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Tomas Vondra]

On 9/5/22 06:32, Amit Kapila wrote:
> On Sun, Sep 4, 2022 at 7:38 PM Tomas Vondra
>  wrote:
>>
>> On 9/4/22 14:24, Tomas Vondra wrote:
>>>
 As per
 my understanding, the problem I reported in the email [1] is the same
 and we have seen this in BF failures as well. I posted a way to
 reproduce it in that email. It seems this is possible if the decoding
 gets XLOG_HEAP2_NEW_CID as the first record (belonging to a
 subtransaction) after XLOG_RUNNING_XACTS.

>>>
>>> Interesting. That's certainly true for WAL in the crashing case:
>>>
>>> rmgr: Standby len (rec/tot): 58/58, tx:  0, lsn:
>>> 0/01CDCD70, prev 0/01CDCD10, desc: RUNNING_XACTS nextXid 850
>>> latestCompletedXid 847 oldestRunningXid 848; 1 xacts: 848
>>> rmgr: Heap2   len (rec/tot): 60/60, tx:849, lsn:
>>> 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel 1663/16384/1249; tid
>>> 58/38; cmin: 1, cmax: 14, combo: 6
>>>
>>
>> I investigated using the pgdata from the crashed run (can provide, if
>> you have rpi4 or some other aarch64 machine), and the reason is pretty
>> simple - the restart_lsn for the slot is 0/1CDCD70, which is looong
>> after the subxact assignment, so we add both xids as toplevel.
>>
>> That seems broken - if we skip the assignment like this, doesn't that
>> break spill-to-disk and/or streaming? IIRC that's exactly why we had to
>> start logging assignments immediately with wal_level=logical.
>>
> 
> We had started logging assignments immediately in commit 0bead9af48
> for streaming transactions in PG14. This issue exists prior to that. I
> have tried and reproduced it in PG13 but I think it will be there even
> before that. So, I am not sure if the spilling behavior is broken due
> to this. I think if we don't get assignment recording before
> processing changes during decoding commit then we could miss sending
> the changes which won't be the case here. Do you see any other
> problem?
> 

I can't, but that's hardly a proof of anything. You're right spilling to
disk may not be broken by this, though. I forgot it precedes assignments
being logged immediately, so it does not rely on that.

>> Or maybe we're not dealing with the restart_lsn properly, and we should
>> have ignored those records. Both xacts started long before the restart
>> LSN, so we're not seeing the whole xact anyway.
>>
> 
> Right, but is that problematic? The restart LSN will be used as a
> point to start reading the WAL and that helps in building a consistent
> snapshot. However, for decoding to send the commit, we use
> start_decoding_at point which will ensure that we send complete
> transactions.
> 

Which part would not be problematic? There's some sort of a bug, that's
for sure.

I think it's mostly clear we won't output this transaction, because the
restart LSN is half-way through. We can either ignore it at commit time,
and then we have to make everything work in case we miss assignments (or
any other part of the transaction).

Or we can ignore stuff early, and not even process some of the changes.
For example in this case do we need to process the NEW_CID contents for
transaction 848? If we can skip that bit, the problem will disappear.

But maybe this is futile and there are other similar issues ...

>> However, when processing the NEW_CID record:
>>
>> tx:849, lsn: 0/01CDCDB0, prev 0/01CDCD70, desc: NEW_CID rel
>> 1663/16384/1249; tid 58/38; cmin: 1, cmax: 14, combo: 6
>>
>> we ultimately do this in SnapBuildProcessNewCid:
>>
>> #1  0x005566cccdb4 in ReorderBufferAddNewTupleCids (rb=0x559dd64218,
>> xid=848, lsn=30264752, locator=..., tid=..., cmin=1, cmax=14,
>> combocid=6) at reorderbuffer.c:3218
>> #2  0x005566cd1f7c in SnapBuildProcessNewCid (builder=0x559dd6a248,
>> xid=849, lsn=30264752, xlrec=0x559dd6e1e0) at snapbuild.c:818
>>
>> so in fact we *know* 849 is a subxact of 848, but we don't call
>> ReorderBufferAssignChild in this case. In fact we can't even do the
>> assignment easily in this case, because we create the subxact first, so
>> that the crash happens right when we attempt to create the toplevel one,
>> and we never even get a chance to do the assignment:
>>
>> 1) process the NEW_CID record, logged for 849 (subxact)
>> 2) process CIDs in the WAL record, which has topleve_xid 848
>>
>>
>> So IMHO we need to figure out what to do for WAL records that create
>> both the toplevel and subxact - either we need to skip them, or rethink
>> how we create the ReorderBufferTXN structs.
>>
> 
> As per my understanding, we can't skip them as they are used to build
> the snapshot.
> 

Don't we know 848 (the top-level xact) won't be decoded? In that case we
won't need the snapshot, so why build it? Of course, the NEW_CID is
logged with xid 849 and we don't know it's subxact of 848, but when
processing the NEW_CID content we can realize that (xl_heap_new_cid does
include top_xid).


regards

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

Re: POC: GROUP BY optimization

[Michael Paquier]

On Mon, Sep 05, 2022 at 12:14:48AM +0200, Tomas Vondra wrote:
> I've pushed the fix to 15+master. In the end I just used David's patches
> that set parallel_setup_cost to 0.

Thanks, Tomas!
--
Michael


signature.asc
Description: PGP signature

Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn", File: "reorderbuffer.c", Line: 927, PID: 568639)

[Amit Kapila]

On Sun, Sep 4, 2022 at 11:10 PM Tomas Vondra
 wrote:
>
> On 9/4/22 16:08, Tomas Vondra wrote:
> > ...
> >
> > so in fact we *know* 849 is a subxact of 848, but we don't call
> > ReorderBufferAssignChild in this case. In fact we can't even do the
> > assignment easily in this case, because we create the subxact first, so
> > that the crash happens right when we attempt to create the toplevel one,
> > and we never even get a chance to do the assignment:
> >
> > 1) process the NEW_CID record, logged for 849 (subxact)
> > 2) process CIDs in the WAL record, which has topleve_xid 848
> >
> >
> > So IMHO we need to figure out what to do for WAL records that create
> > both the toplevel and subxact - either we need to skip them, or rethink
> > how we create the ReorderBufferTXN structs.
> >
>
> This fixes the crash for me, by adding a ReorderBufferAssignChild call
> to SnapBuildProcessNewCid, and tweaking ReorderBufferAssignChild to
> ensure we don't try to create the top xact before updating the subxact
> and removing it from the toplevel_by_lsn list.
>
> Essentially, what's happening is this:
>
> 1) We read the NEW_CID record, which is logged with XID 849, i.e. the
> subxact. But we don't know it's a subxact, so we create it as a
> top-level xact with the LSN.
>
> 2) We start processing contents of the NEW_CID, which however has info
> that 849 is subxact of 848, calls ReorderBufferAddNewTupleCids which
> promptly does ReorderBufferTXNByXid() with the top-level XID, which
> creates it with the same LSN, and crashes because of the assert.
>
> I'm not sure what's the right/proper way to fix this ...
>
> The problem is ReorderBufferAssignChild was coded in a way that did not
> expect the subxact to be created first (as a top-level xact).
>

I think there was a previously hard-coded way to detect that and we
have removed it in commit
(https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e3ff789acfb2754cd7b5e87f6f4463fd08e35996).
I think it is possible that subtransaction gets logged without
previous top-level txn record as shown in the commit shared.

-- 
With Regards,
Amit Kapila.

Re: Asynchronous execution support for Custom Scan

[Etsuro Fujita]

On Fri, Sep 2, 2022 at 10:43 PM Kazutaka Onishi  wrote:
> The asynchronous version "ctidscan" plugin is ready.

Thanks for that!

I looked at the extended version quickly.  IIUC, it uses the proposed
APIs, but actually executes ctidscans *synchronously*, so it does not
improve performance.  Right?

Anyway, that version seems to be useful for testing that the proposed
APIs works well.  So I'll review the proposed patches with it.  I'm
not Fujii-san, though.  :-)

Best regards,
Etsuro Fujita

Re: Refactoring postgres_fdw/connection.c

[Etsuro Fujita]

On Tue, Jul 26, 2022 at 4:25 PM Kyotaro Horiguchi
 wrote:
> At Tue, 26 Jul 2022 00:54:47 +0900, Fujii Masao  
> wrote in
> > There are two functions, pgfdw_get_result() and
> > pgfdw_get_cleanup_result(),
> > to get a query result. They have almost the same code, call
> > PQisBusy(),
> > WaitLatchOrSocket(), PQconsumeInput() and PQgetResult() in the loop,
> > but only pgfdw_get_cleanup_result() allows its callers to specify the
> > timeout.
> > 0001 patch transforms pgfdw_get_cleanup_result() to the common
> > function
> > to get a query result and makes pgfdw_get_result() use it instead of
> > its own (duplicate) code. The patch also renames
> > pgfdw_get_cleanup_result()
> > to pgfdw_get_result_timed().
>
> Agree to that refactoring.

+1 for that refactoring.  Here are a few comments about the 0001 patch:

I'm not sure it's a good idea to change the function's name, because
that would make backpatching hard.  To avoid that, how about
introducing a workhorse function for pgfdw_get_result and
pgfdw_get_cleanup_result, based on the latter function as you
proposed, and modifying the two functions so that they call the
workhorse function?

@@ -1599,13 +1572,9 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
entry = (ConnCacheEntry *) lfirst(lc);

/* Ignore errors (see notes in pgfdw_xact_callback) */
-   while ((res = PQgetResult(entry->conn)) != NULL)
-   {
-   PQclear(res);
-   /* Stop if the connection is lost (else we'll loop infinitely) */
-   if (PQstatus(entry->conn) == CONNECTION_BAD)
-   break;
-   }
+   pgfdw_get_result_timed(entry->conn, 0, , NULL);
+   PQclear(res);

The existing code prevents interruption, but this would change to
allow it.  Do we need this change?

> > pgfdw_xact_callback() and pgfdw_subxact_callback() have similar codes
> > to
> > issue COMMIT or RELEASE SAVEPOINT commands. 0002 patch adds the common
> > function,
> > pgfdw_exec_pre_commit(), for that purpose, and changes those functions
> > so that they use the common one.
>
> I'm not sure the two are similar with each other.  The new function
> pgfdw_exec_pre_commit() looks like a merger of two isolated code paths
> intended to share a seven-line codelet.  I feel the code gets a bit
> harder to understand after the change.  I mildly oppose to this part.

I have to agree with Horiguchi-san, because as mentioned by him, 1)
there isn't enough duplicate code in the two bits to justify merging
them into a single function, and 2) the 0002 patch rather just makes
code complicated.  The current implementation is easy to understand,
so I'd vote for leaving them alone for now.

(I think the introduction of pgfdw_abort_cleanup is good, because that
de-duplicated much code that existed both in pgfdw_xact_callback and
in pgfdw_subxact_callback, which would outweigh the downside of
pgfdw_abort_cleanup that it made code somewhat complicated due to the
logic to handle both the transaction and subtransaction cases within
that single function.  But 0002 is not the case, I think.)

> > pgfdw_finish_pre_commit_cleanup() and
> > pgfdw_finish_pre_subcommit_cleanup()
> > have similar codes to wait for the results of COMMIT or RELEASE
> > SAVEPOINT commands.
> > 0003 patch adds the common function, pgfdw_finish_pre_commit(), for
> > that purpose,
> > and replaces those functions with the common one.
> > That is, pgfdw_finish_pre_commit_cleanup() and
> > pgfdw_finish_pre_subcommit_cleanup()
> > are no longer necessary and 0003 patch removes them.
>
> It gives the same feeling with 0002.

I have to agree with Horiguchi-san on this as well; the existing
single-purpose functions are easy to understand, so I'd vote for
leaving them alone.

Sorry for being late to the party.

Best regards,
Etsuro Fujita

Re: SQL/JSON features for v15

[Amit Langote]

On Fri, Sep 2, 2022 at 8:56 PM Justin Pryzby  wrote:
> On Wed, Aug 31, 2022 at 03:51:18PM +0900, Amit Langote wrote:
> > Finally, I get this warning:
> >
> > execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
> > execExprInterp.c:4765:3: warning: missing braces around initializer
> > [-Wmissing-braces]
> >NameData encoding = {0};
> >^
> > execExprInterp.c:4765:3: warning: (near initialization for
> > ‘encoding.data’) [-Wmissing-braces]
>
> With what compiler ?
>
> This has came up before:
> 20211202033145.gk17...@telsasoft.com
> 20220716115932.gv18...@telsasoft.com

Didn't realize it when I was reviewing the patch but somehow my build
script had started using gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44),
which I know is old.

- Amit