Re: Query Jumbling for CALL and SET utility statements

[Michael Paquier]

On Mon, Sep 19, 2022 at 08:29:22AM +0200, Drouvot, Bertrand wrote:
> Please find attached v6 taking care of the remarks mentioned above.
> +case T_VariableSetStmt:
> +{
> +VariableSetStmt *stmt = (VariableSetStmt *) node;
> +
> +/* stmt->name is NULL for RESET ALL */
> +if (stmt->name)
> +{
> +APP_JUMB(stmt->kind);
> +APP_JUMB_STRING(stmt->name);
> +JumbleExpr(jstate, (Node *) stmt->args);
> +}
> +}
> +break;

Hmm.  If VariableSetStmt->is_local is not added to the jumble, then
aren't "SET foo = $1" and "SET LOCAL foo = $1" counted as the same
query? 

I am not seeing SAVEPOINT, RELEASE, ROLLBACK .. TO SAVEPOINT
mentioned on this thread.  Would these be worth considering in what
gets compiled?  That would cover the remaining bits of
TransactionStmt.  The ODBC driver abuses of savepoints, for example,
so this could be useful for monitoring purposes in such cases.

As of the code stands, it could be cleaner to check
IsJumbleUtilityAllowed() in compute_utility_query_id(), falling back 
to a default in JumbleQuery().  Not that what your patch does is
incorrect, of course.
--
Michael


signature.asc
Description: PGP signature

Re: Add last failed connection error message to pg_stat_wal_receiver

[Bharath Rupireddy]

On Thu, Aug 18, 2022 at 9:01 AM Michael Paquier  wrote:
>
> PANIC is not something you'd care about as the system would go down as
> and shared memory would be reset (right?) even if restart_on_crash is
> enabled.  Perhaps it would help here to use something like a macro to
> catch and save the error, in a style similar to what's in hba.c for
> example, which is the closest example I can think of, even if on ERROR
> we don't really care about the error string anyway as there is nothing
> to report back to the SQL views used for the HBA/ident files.
>
> FATAL may prove to be tricky though, because I'd expect the error to
> be saved in shared memory in this case.  This is particularly critical
> as this takes the WAL receiver process down, actually.

Hm, we can use error callbacks or pg try/catch blocks to save the
error message into walreceiver shared memory.

> Anyway, outside the potential scope of the proposal, there are more
> things that I find strange with the code:
> - Why isn't the string reset when the WAL receiver is starting up?
> That surely is not OK to keep a past state not referring to what
> actually happens with a receiver currently running.

I agree that it's not a good way to show some past failure state when
things are fine currently. Would naming the column name as
last_connectivity_error or something better and describing it in the
docs clearly help here?

Otherwise, we can have another simple function that just returns the
last connection failure of walreceiver and if required PID.

> - pg_stat_wal_receiver (system view) reports no rows if pid is NULL,
> which would be the state stored in shared memory after a connection.
> This means that one would never be able to see last_conn_error except
> when calling directly the SQL function pg_stat_get_wal_receiver().
>
> One could say that we should report a row for this view all the time,
> but this creates a compatibility breakage: existing application
> assuming something like (one row <=> WAL receiver running) could
> break.

-1.

We can think of having a separate infrastructure for reporting all
backend or process specific errors similar to pg_stat_activity and
pg_stat_get_activity, but that needs some shared memory and all of
that - IMO, it's an overkill.

I'm fine to withdraw this thread, if none of the above thoughts is
sensible enough to pursue further.

Thoughts?

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

Re: document the need to analyze partitioned tables

[Andrey Lepikhov]

On 10/5/22 13:37, Laurenz Albe wrote:

On Mon, 2022-03-28 at 15:05 +0200, Tomas Vondra wrote:

I've pushed the last version, and backpatched it to 10 (not sure I'd
call it a bugfix, but I certainly agree with Justin it's worth
mentioning in the docs, even on older branches).


I'd like to suggest an improvement to this.  The current wording could
be read to mean that dead tuples won't get cleaned up in partitioned tables.


By the way, where are the statistics of a partitioned tables used?  The actual
tables scanned are always the partitions, and in the execution plans that
I have seen, the optimizer always used the statistics of the partitions.

For example, it is used to estimate selectivity of join clause:

CREATE TABLE test (id integer, val integer) PARTITION BY hash (id);
CREATE TABLE test_0 PARTITION OF test
  FOR VALUES WITH (modulus 2, remainder 0);
CREATE TABLE test_1 PARTITION OF test
  FOR VALUES WITH (modulus 2, remainder 1);

INSERT INTO test (SELECT q, q FROM generate_series(1,10) AS q);
VACUUM ANALYZE test;
INSERT INTO test (SELECT q, q%2 FROM generate_series(11,200) AS q);
VACUUM ANALYZE test_0,test_1;

EXPLAIN (ANALYZE, TIMING OFF, SUMMARY OFF)
SELECT * FROM test t1, test t2 WHERE t1.id = t2.val;
VACUUM ANALYZE test;
EXPLAIN (ANALYZE, TIMING OFF, SUMMARY OFF)
SELECT * FROM test t1, test t2 WHERE t1.id = t2.val;

Here without actual statistics on parent table we make wrong prediction.

--
Regards
Andrey Lepikhov
Postgres Professional

Re: pg_statsinfo - PG15 support?

[Kyotaro Horiguchi]

At Thu, 6 Oct 2022 09:34:02 +1100, Greg Nancarrow  wrote 
in 
> To the developer(s) who work(s) on pg_statsinfo, are there plans to
> support PG15 and when might pg_statsinfo v15 source be released?
> I can see that for v14 of pg_statsinfo there is an incompatibility
> with the PG15 autovacuum log (i.e. in PG15 some existing autovacuum
> log fields have been removed and some new fields have been added). I
> also noticed that PG15 changes how shared libraries must request
> additional shared memory during initialization and pg_statsinfo source
> code would need updating for this.

Thank you for the info and apologize for the inconvenience. "We" are
aware of the points of incompatibility with PG15 and now working on
pg_statsinfo for PG15. For our historical and procedural reasons, it
is not fully open-sourced and it is usually released around March of
the year following the release of the corresponding version of
PostgreSQL and the source files are published at the same time.  Thus
pg_statsinfo for PG15 will be released in March, 2023.

Thank you for your interest.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: START_REPLICATION SLOT causing a crash in an assert build

[Kyotaro Horiguchi]

At Thu, 6 Oct 2022 13:44:43 +0900, Michael Paquier  wrote 
in 
> On Wed, Oct 05, 2022 at 11:24:57PM -0400, Jonathan S. Katz wrote:
> > On 10/5/22 8:44 PM, Andres Freund wrote:
> >> I have two ideas how to fix it. As a design constraint, I'd be interested 
> >> in
> >> the RMTs opinion on the following:
> >> Is a cleaner fix that changes the stats format (i.e. existing stats will be
> >> thrown away when upgrading) or one that doesn't change the stats format
> >> preferrable?
> > 
> > [My opinion, will let Michael/John chime in]
> > 
> > Ideally we would keep the stats on upgrade -- I think that's the better user
> > experience.
> 
> The release has not happened yet, so I would be fine to remain
> flexible and bump again PGSTAT_FILE_FORMAT_ID so as we have the
> cleanest approach in place for the release and the future.  At the
> end, we would throw automatically the data of a file that's marked
> with a version that does not match with what we expect at load time,
> so there's a limited impact on the user experience, except, well,
> losing these stats, of course.

+1. FWIW, the atttached is an example of what it looks like if we
avoid file format change.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 4effe9279d6535191d7f5478bb76490dc8762d33 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 6 Oct 2022 14:06:04 +0900
Subject: [PATCH v2] If we avoid file format

---
 src/backend/utils/activity/pgstat.c  |  9 +++--
 src/backend/utils/activity/pgstat_replslot.c | 18 --
 src/include/pgstat.h |  1 -
 src/include/utils/pgstat_internal.h  |  3 +++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 609f0b1ad8..a03184c9ef 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -310,6 +310,7 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
 
 		.reset_timestamp_cb = pgstat_replslot_reset_timestamp_cb,
 		.to_serialized_name = pgstat_replslot_to_serialized_name_cb,
+		.set_name = pgstat_replslot_set_name_cb,
 		.from_serialized_name = pgstat_replslot_from_serialized_name_cb,
 	},
 
@@ -1523,6 +1524,8 @@ pgstat_read_statsfile(void)
 	PgStat_HashKey key;
 	PgStatShared_HashEntry *p;
 	PgStatShared_Common *header;
+	const PgStat_KindInfo *kind_info = NULL;
+	NameData	name = {0};
 
 	CHECK_FOR_INTERRUPTS();
 
@@ -1538,9 +1541,7 @@ pgstat_read_statsfile(void)
 	else
 	{
 		/* stats entry identified by name on disk (e.g. slots) */
-		const PgStat_KindInfo *kind_info = NULL;
 		PgStat_Kind kind;
-		NameData	name;
 
 		if (!read_chunk_s(fpin, &kind))
 			goto error;
@@ -1590,6 +1591,10 @@ pgstat_read_statsfile(void)
 	pgstat_get_entry_len(key.kind)))
 		goto error;
 
+	/* set name if it requires to do that separately */
+	if (kind_info && kind_info->set_name && NameStr(name)[0])
+		kind_info->set_name(header, &name);
+		
 	break;
 }
 			case 'E':
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 2039ac3147..9fa4df610d 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -86,7 +86,7 @@ pgstat_report_replslot(ReplicationSlot *slot, const PgStat_StatReplSlotEntry *re
 	 * Any mismatch should have been fixed in pgstat_create_replslot() or
 	 * pgstat_acquire_replslot().
 	 */
-	Assert(namestrcmp(&statent->slotname, NameStr(slot->data.name)) == 0);
+	Assert(namestrcmp(&shstatent->slotname, NameStr(slot->data.name)) == 0);
 
 	/* Update the replication slot statistics */
 #define REPLSLOT_ACC(fld) statent->fld += repSlotStat->fld
@@ -124,7 +124,7 @@ pgstat_create_replslot(ReplicationSlot *slot)
 	 * if we previously crashed after dropping a slot.
 	 */
 	memset(&shstatent->stats, 0, sizeof(shstatent->stats));
-	namestrcpy(&shstatent->stats.slotname, NameStr(slot->data.name));
+	namestrcpy(&shstatent->slotname, NameStr(slot->data.name));
 
 	pgstat_unlock_entry(entry_ref);
 }
@@ -148,11 +148,11 @@ pgstat_acquire_replslot(ReplicationSlot *slot)
 	 * NB: need to accept that there might be stats from an older slot, e.g.
 	 * if we previously crashed after dropping a slot.
 	 */
-	if (NameStr(statent->slotname)[0] == 0 ||
-		namestrcmp(&statent->slotname, NameStr(slot->data.name)) != 0)
+	if (NameStr(shstatent->slotname)[0] == 0 ||
+		namestrcmp(&shstatent->slotname, NameStr(slot->data.name)) != 0)
 	{
 		memset(statent, 0, sizeof(*statent));
-		namestrcpy(&statent->slotname, NameStr(slot->data.name));
+		namestrcpy(&shstatent->slotname, NameStr(slot->data.name));
 	}
 
 	pgstat_unlock_entry(entry_ref);
@@ -187,7 +187,13 @@ pgstat_fetch_replslot(NameData slotname)
 void
 pgstat_replslot_to_serialized_name_cb(const PgStatShared_Common *header, NameData *name)
 {
-	namestrcpy(name, Name

Re: Issue with pg_stat_subscription_stats

[Michael Paquier]

On Tue, Jul 12, 2022 at 09:31:16AM +0530, Amit Kapila wrote:
> I am not against backpatching this but OTOH it doesn't appear critical
> enough to block one's work, so not backpatching should be fine.

We are just talking about the reset timestamp not being set at
when the object is created, right?  This does not strike me as
critical, so applying it only on HEAD is fine IMO.  A few months ago,
while in beta, I would have been fine with something applied to
REL_15_STABLE.  Now that we are in RC, that's not worth taking a risk
in my opinion.

Amit or Andres, are you planning to double-check and perhaps merge
this patch to take care of the inconsistency?
--
Michael


signature.asc
Description: PGP signature

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

[David Rowley]

On Wed, 5 Oct 2022 at 04:55, Tom Lane  wrote:
> After studying the existing usages of MemoryContextContains, I think
> there is a better answer, which is to just nuke them.

I was under the impression you wanted to keep that function around in
cassert builds for some of the guc.c changes you were making.

> As far as I can tell, the datumCopy steps associated with aggregate
> finalfns are basically useless.  They only serve to prevent
> returning a pointer directly to the aggregate's transition value
> (or, perhaps, to a portion thereof).  But what's wrong with that?
> It'll last as long as we need it to.  Maybe there was a reason
> back before we required finalfns to not scribble on the transition
> values, but those days are gone.

Yeah, I wondered the same thing. I couldn't see a situation where the
aggregate context would disappear.

> The same goes for aggregate serialfns --- although there, I can't
> avoid the feeling that the datumCopy step was just cargo-culted in.
> I don't think there can exist a serialfn that doesn't return a
> freshly-palloced bytea.

Most likely. I probably copied that as I wouldn't have understood why
we did any copying when calling the finalfn. I still don't understand
why. Seems there's no good reason if we're both in favour of removing
it.

> The one place where we actually need the conditional datumCopy is
> with window functions, and even there I don't think we need it
> in simple cases with only one window function.  The case that is
> hazardous is where multiple window functions are sharing a
> WindowObject.  So I'm content to optimize the single-window-function
> case and just always copy if there's more than one.  (Sadly, there
> is no existing regression test that catches this, so I added one.)

I was unsure what window functions might exist out in the wild, so I'd
added some code to pass along the return type information so that any
extensions which need to make a copy can do so.  However, maybe it's
better just to wait to see if anyone complains about that before we go
to the trouble.

I've looked at your patches and don't see any problems. Our findings
seem to be roughly the same. i.e the datumCopy is mostly useless.
However, you've noticed the requirement to datumCopy when there are
multiple window functions using the same window along with yours
containing the call to MakeExpandedObjectReadOnly() where I missed
that.

This should also slightly improve the performance of LEAD and LAG with
byref types, which seems like a good side-effect.

I guess the commit message for 0002 should mention that for pointers
to allocated chunks that GetMemoryChunkContext() can be used in place
of MemoryContextContains(). I did see that PostGIS does use
MemoryContextContains(), though I didn't look at their code to figure
out if they're always passing it a pointer to an allocated chunk.
Maybe it's worth doing;

#define MemoryContextContains(c, p) (GetMemoryChunkContext(p) == (c))

in memutils.h? or are we better to force extension authors to
re-evaluate their code in case anyone is passing memory that's not
pointing directly to a palloc'd chunk?

David

Re: installcheck-world concurrency issues

[Michael Paquier]

On Tue, Oct 04, 2022 at 11:35:53AM -0700, Andres Freund wrote:
> On 2022-10-04 17:05:40 +0900, Michael Paquier wrote:
>> I am still studying a lot of this area, but it seems like all the
>> spots requiring a custom configuration (aka NO_INSTALLCHECK) are
>> covered.  --setup running is working here with 0003.
> 
> Thanks for checking.

For the archives' sake: this has been applied as of 6a20b04 and
c3315a7.
--
Michael


signature.asc
Description: PGP signature

Re: START_REPLICATION SLOT causing a crash in an assert build

[Michael Paquier]

On Wed, Oct 05, 2022 at 11:24:57PM -0400, Jonathan S. Katz wrote:
> On 10/5/22 8:44 PM, Andres Freund wrote:
>> I have two ideas how to fix it. As a design constraint, I'd be interested in
>> the RMTs opinion on the following:
>> Is a cleaner fix that changes the stats format (i.e. existing stats will be
>> thrown away when upgrading) or one that doesn't change the stats format
>> preferrable?
> 
> [My opinion, will let Michael/John chime in]
> 
> Ideally we would keep the stats on upgrade -- I think that's the better user
> experience.

The release has not happened yet, so I would be fine to remain
flexible and bump again PGSTAT_FILE_FORMAT_ID so as we have the
cleanest approach in place for the release and the future.  At the
end, we would throw automatically the data of a file that's marked
with a version that does not match with what we expect at load time,
so there's a limited impact on the user experience, except, well,
losing these stats, of course.
--
Michael


signature.asc
Description: PGP signature

Re: shadow variables - pg15 edition

[David Rowley]

On Thu, 6 Oct 2022 at 13:39, Andres Freund  wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.

Thanks for writing that and for looking at the patch.

FWIW, I'm +1 for having this part of our default compilation flags.  I
don't want to have to revisit this on a yearly basis. I imagine Justin
doesn't want to do that either.  I feel that since this work has
already uncovered 2 existing bugs that it's worth having this as a
default compilation flag.  Additionally, in the cases like in the
PLy_exec_trigger() trigger case below, I feel this has resulted in
slightly more simple code that's easier to follow. I feel having to be
slightly more inventive with variable names in a small number of cases
is worth the trouble.  I feel the cases where this could get annoying
are probably limited to variables declared in macros. Maybe that's
just a reason to consider static inline functions instead.  That
wouldn't work for macros such as PG_TRY(), but I think macros in that
category are rare.  I think switching it on does not mean we can never
switch it off again should we ever find something we're unable to work
around.  That just seems a little unlikely given that with the prior
commits plus the attached patch, we've managed to fix ~30 years worth
of opportunity to introduce shadowed local variables.

> > diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> >  #define HS_FINALIZE(hsp_,count_,buf_,ptr_) 
> >   \
> >   do {  
> >   \
> > - int buflen = (ptr_) - (buf_); 
> >   \
> > + int _buflen = (ptr_) - (buf_);
> >   \
>
> Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
> we could just remove the local?

You're right. It's not that pretty, but I don't feel like making the
hazards any worse is a good idea. This is old code. I'd rather change
it as little as possible to minimise the risk of introducing any bugs.
I'm open to other names for the variable, but I just don't want to
widen the scope for multiple evaluation hazards.

> > --- a/src/interfaces/libpq/fe-secure-gssapi.c
> > +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> > @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t 
> > len)
> > - ssize_t ret;
> > + ssize_t retval;
>
> That looks like it could easily lead to confusion further down the
> line. Wouldn't the better fix here be to remove the inner variable?

hmm. You're maybe able to see something I can't there, but to me, it
looks like reusing the outer variable could change the behaviour of
the function.  Note at the end of the function we set "ret" just
before the goto label.  It looks like it might be possible for the
goto to jump to the point after "ret = bytes_sent;", in which case we
should return -1, the default value for the outer "ret". If I go and
reuse the outer "ret" for something else then it'll return whatever
value it's left set to.  I could study the code more and perhaps work
out that that cannot happen, but if it can't then it's really not
obvious to me and if it's not obvious then I just don't feel the need
to take any undue risks by reusing the outer variable.  I'm open to
better names, but I'd just rather not reuse the outer scoped variable.

> > --- a/src/pl/plpython/plpy_exec.c
> > +++ b/src/pl/plpython/plpy_exec.c
> > @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, 
> > PLyProcedure *proc)
> > - TriggerData *tdata = (TriggerData *) 
> > fcinfo->context;
> > + TriggerData *trigdata = (TriggerData *) 
> > fcinfo->context;

> This doesn't strike me as a good fix either. Isn't the inner tdata exactly
> the same as the outer data?

Yeah, you're right. I've adjusted the patch to use the outer scoped
variable and get rid of the inner scoped one.

> > --- a/src/test/modules/test_integerset/test_integerset.c
> > +++ b/src/test/modules/test_integerset/test_integerset.c
> > @@ -585,26 +585,26 @@ test_huge_distances(void)
>
> This is one of the cases where our insistence on -Wdeclaration-after-statement
> really makes this unnecessary ugly... Declaring x at the start of the function
> just makes this harder to read.

Yeah, it's not pretty. Maybe one day we'll relax that rule. Until
then, I think it's not worth expending too much thought on a test
module.

David
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index e64291e049..dd26d6ac29 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -232,8 +232,6 @@ blinsert(Relation index, Datum *values, bool *isnull,
 
if (metaData->nEnd > metaData->nStart)
{
-   Page

Re: START_REPLICATION SLOT causing a crash in an assert build

[Jonathan S. Katz]

On 10/5/22 8:44 PM, Andres Freund wrote:

Hi,

On 2022-10-05 13:00:53 -0400, Jonathan S. Katz wrote:

On 9/27/22 1:52 AM, Kyotaro Horiguchi wrote:

Thanks!

At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund  wrote in

I wonder if the correct fix here wouldn't be to move the slotname out of
PgStat_StatReplSlotEntry?


Ugh. Right. I thought its outer struct as purely the part for the
common header. But we can freely place anything after the header
part. I moved it to the outer struct. I didn't clear that part in
pgstat_create_relation() because it is filled in immediately.

The attached is that.


This is still listed as an open item[1] for v15. Does this fix proposed
address the issue?


Unfortunately not - it doesn't even pass the existing tests
(test_decoding/001_repl_stats fails) :(.


Thanks for checking.


The reason for that is that with the patch nothing restores the slotname when
reading stats from disk. That turns out not to cause immediate issues, but at
the next shutdown the name won't be set, and we'll serialize the stats data
with an empty string as the name.


Ah.


I have two ideas how to fix it. As a design constraint, I'd be interested in
the RMTs opinion on the following:
Is a cleaner fix that changes the stats format (i.e. existing stats will be
thrown away when upgrading) or one that doesn't change the stats format
preferrable?


[My opinion, will let Michael/John chime in]

Ideally we would keep the stats on upgrade -- I think that's the better 
user experience.


Thanks,

Jonathan



OpenPGP_signature
Description: OpenPGP digital signature

[PATCH v1] [meson] fix some typo to make it more readable

[Junwang Zhao]

Hi Andres,

Seems there are some typo in file src/backend/meson.build comment, pls
have a look.

-- 
Regards
Junwang Zhao


0001-meson-fix-some-typo-to-make-it-more-readable.patch
Description: Binary data

RE: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

[kuroda.hay...@fujitsu.com]

Dear Önder,

Thank you for updating the patch! At first I replied to your comments.

> My thinking on those functions is that they should probably stay
> in src/backend/replication/logical/relation.c. My main motivation is that
> those functions are so much tailored to the purposes of this file that I
> cannot see any use-case for these functions in any other context.

I was not sure what should be, but I agreed that functions will be not used 
from other parts.

> Hmm, I cannot follow this comment. Can you please clarify?

In your patch:

```
+   /* Simple case, we already have a primary key or a replica identity 
index */
+   idxoid = GetRelationIdentityOrPK(localrel);
+   if (OidIsValid(idxoid))
+   return idxoid;
+
+   /*
+* If index scans are disabled, use a sequential scan.
+*
+* Note that we still allowed index scans above when there is a primary
+* key or unique index replica identity, but that is the legacy 
behaviour
+* (even when enable_indexscan is false), so we hesitate to move this
+* enable_indexscan check to be done earlier in this function.
+*/ 
```

And the paragraph " Note that we..." should be at above of 
GetRelationIdentityOrPK().
Future readers will read the function from top to bottom,
and when they read around GetRelationIdentityOrPK() they may be confused.

> So, I think it is better to have specific names, no?

OK.

> The inlined comment in the function has a similar comment. Is that clear
> enough?
> /* * Generate restrictions for all columns in the form of col_1 = $1 AND *
> col_2 = $2 ... */

Actually I missed it, but I still think that whole of emulated SQL should be 
clarified. 

> Though, I agree that we can improve the code a bit. I now
> use targetrelkind and dropped localrelid to check whether the target is a
> partitioned table. Is this better?

Great improvement. Genus!

> Well, I'm not sure if it is worth the complexity. There are only 4 usages
> of the same table, and these are all pretty simple statements, and all
> other tests seem to have a similar pattern. I have not seen any tests where
> these simple statements are done in a function even if they are repeated.
> I'd rather keep it so that this doesn't lead to other style discussions?

If other tests do not combine such parts, it's OK.
My motivation of these comments were to reduce the number of row for the test 
code.

> Oh, I didn't know about this, thanks!

Now meson test system do your test. OK.


And followings are the comments for v14. They are mainly about comments.

===
01. relation.c - logicalrep_rel_open

```
+   /*
+* Finding a usable index is an infrequent task. It occurs when 
an
+* operation is first performed on the relation, or after 
invalidation
+* of the relation cache entry (e.g., such as ANALYZE).
+*/
+   entry->usableIndexOid = 
FindLogicalRepUsableIndex(entry->localrel, remoterel);
```

I thought you can mention CREATE INDEX in the comment.

According to your analysis [1] the relation cache will be invalidated if users 
do CREATE INDEX
At that time the hash entry will be removed (logicalrep_relmap_invalidate_cb) 
and "usable" index
will be checked again.

~~~
02. relation.c - logicalrep_partition_open

```
+   /*
+* Finding a usable index is an infrequent task. It occurs when an
+* operation is first performed on the relation, or after invalidation 
of
+* the relation cache entry (e.g., such as ANALYZE).
+*/
+   entry->usableIndexOid = FindLogicalRepUsableIndex(partrel, remoterel);
+
```

Same as above

~~~
03. relation.c - GetIndexOidFromPath

```
+   if (path->pathtype == T_IndexScan || path->pathtype == T_IndexOnlyScan)
+   {
+   IndexPath  *index_sc = (IndexPath *) path;
+
+   return index_sc->indexinfo->indexoid;
+   }
```

I thought Assert(OidIsValid(indexoid)) may be added here. Or is it quite 
trivial?

~~~
04. relation.c - IndexOnlyOnExpression

This method just returns "yes" or "no", so the name of method should be start 
"Has" or "Is".

~~~
05. relation.c - SuitablePathsForRepIdentFull

```
+/*
+ * Iterates over the input path list and returns another
+ * path list that includes index [only] scans where paths
+ * with non-btree indexes, partial indexes or
+ * indexes on only expressions have been removed.
+ */
```

These lines seems to be around 60 columns. Could you expand around 80?

~~~
06. relation.c - SuitablePathsForRepIdentFull

```
+   RelationindexRelation;
+   IndexInfo  *indexInfo;
+   boolis_btree;
+   boolis_partial;
+   boolis_only_on_expression;
+
+   indexRelation = index_open(idxoid, AccessShareLock);
+   indexInfo = BuildIndexInfo(indexR

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

[Michael Paquier]

On Wed, Oct 05, 2022 at 03:32:20PM +0200, Drouvot, Bertrand wrote:
> On 10/5/22 9:24 AM, Michael Paquier wrote:
>> - test_role() -> test_conn() to be able to pass down a database name.
>> - reset_pg_hba() to control the host, db and user parts.  The host
>> part does not really apply after moving the hosts checks to a more
>> secure location, so I guess that this had better be extended just for
>> the user and database, keeping host=local all the time.
>> I am planning to apply 0001 attached independently,
> 
> 0001 looks good to me.

Thanks.  I have applied this refactoring, leaving the host part out of
the equation as we should rely only on local connections for this
part of the test.  The best fit I can think about for the checks on
the hostname patterns would be either the ssl, ldap or krb5 tests.
SSL is more widely tested than the two others.

>> reducing the
>> footprint of 0002, which is your previous patch left untouched
>> (mostly!).
> 
> Thanks! I'll look at it and the comments you just made up-thread.

Cool, thanks.  One thing that matters a lot IMO (that I forgot to
mention previously) is to preserve the order of the items parsed from
the configuration files.

Also, I am wondering whether we'd better have some regression tests
where a regex includes a comma and a role name itself has a comma,
actually, just to stress more the parsing of individual items in the
HBA file.
--
Michael


signature.asc
Description: PGP signature

Re: START_REPLICATION SLOT causing a crash in an assert build

[Andres Freund]

Hi,

On 2022-10-05 13:00:53 -0400, Jonathan S. Katz wrote:
> On 9/27/22 1:52 AM, Kyotaro Horiguchi wrote:
> > Thanks!
> > 
> > At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund  
> > wrote in
> > > I wonder if the correct fix here wouldn't be to move the slotname out of
> > > PgStat_StatReplSlotEntry?
> > 
> > Ugh. Right. I thought its outer struct as purely the part for the
> > common header. But we can freely place anything after the header
> > part. I moved it to the outer struct. I didn't clear that part in
> > pgstat_create_relation() because it is filled in immediately.
> > 
> > The attached is that.
> 
> This is still listed as an open item[1] for v15. Does this fix proposed
> address the issue?

Unfortunately not - it doesn't even pass the existing tests
(test_decoding/001_repl_stats fails) :(.

The reason for that is that with the patch nothing restores the slotname when
reading stats from disk. That turns out not to cause immediate issues, but at
the next shutdown the name won't be set, and we'll serialize the stats data
with an empty string as the name.

I have two ideas how to fix it. As a design constraint, I'd be interested in
the RMTs opinion on the following:
Is a cleaner fix that changes the stats format (i.e. existing stats will be
thrown away when upgrading) or one that doesn't change the stats format
preferrable?

Greetings,

Andres Freund

Re: shadow variables - pg15 edition

[Andres Freund]

Hi,

On 2022-10-06 13:00:41 +1300, David Rowley wrote:
> Here's a patch which (I think) fixes the ones I missed.

Yep, does the trick for me.

I attached a patch to add -Wshadow=compatible-local to our set of warnings.


> diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> index 4713e6ea7a..897af244a4 100644
> --- a/contrib/hstore/hstore.h
> +++ b/contrib/hstore/hstore.h
> @@ -128,15 +128,15 @@ typedef struct
>  /* finalize a newly-constructed hstore */
>  #define HS_FINALIZE(hsp_,count_,buf_,ptr_)   
> \
>   do {
> \
> - int buflen = (ptr_) - (buf_);   
> \
> + int _buflen = (ptr_) - (buf_);  
> \

Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
we could just remove the local?



> --- a/src/interfaces/libpq/fe-secure-gssapi.c
> +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
>*/
>   if (PqGSSSendLength)
>   {
> - ssize_t ret;
> + ssize_t retval;

That looks like it could easily lead to confusion further down the
line. Wouldn't the better fix here be to remove the inner variable?


> --- a/src/pl/plpython/plpy_exec.c
> +++ b/src/pl/plpython/plpy_exec.c
> @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure 
> *proc)
>   rv = NULL;
>   else if (pg_strcasecmp(srv, "MODIFY") == 0)
>   {
> - TriggerData *tdata = (TriggerData *) 
> fcinfo->context;
> + TriggerData *trigdata = (TriggerData *) 
> fcinfo->context;
>  
> - if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event) ||
> - 
> TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
> - rv = PLy_modify_tuple(proc, plargs, 
> tdata, rv);
> + if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) 
> ||
> + 
> TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
> + rv = PLy_modify_tuple(proc, plargs, 
> trigdata, rv);
>   else
>   ereport(WARNING,
>   (errmsg("PL/Python 
> trigger function returned \"MODIFY\" in a DELETE trigger -- ignored")));

This doesn't strike me as a good fix either. Isn't the inner tdata exactly
the same as the outer tdata?

tdata = (TriggerData *) fcinfo->context;
...
TriggerData *trigdata = (TriggerData *) 
fcinfo->context;



> --- a/src/test/modules/test_integerset/test_integerset.c
> +++ b/src/test/modules/test_integerset/test_integerset.c
> @@ -585,26 +585,26 @@ test_huge_distances(void)

This is one of the cases where our insistence on -Wdeclaration-after-statement
really makes this unnecessary ugly... Declaring x at the start of the function
just makes this harder to read.

Anyway, this isn't important code, and your fix seem ok.


Greetings,

Andres Freund
diff --git i/configure w/configure
index 1caca21b625..a5a03f6cec3 100755
--- i/configure
+++ w/configure
@@ -5852,6 +5852,97 @@ if test x"$pgac_cv_prog_CXX_cxxflags__Wcast_function_type" = x"yes"; then
 fi
 
 
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wshadow=compatible-local, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Wshadow=compatible-local, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Wshadow_compatible_local+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Wshadow=compatible-local"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Wshadow_compatible_local=yes
+else
+  pgac_cv_prog_CC_cflags__Wshadow_compatible_local=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Wshadow_compatible_local" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Wshadow_compatible_local" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Wshadow_compatible_local" = x"yes"; then
+  CFLAGS="${CFLAGS} -Wshadow=compatible-local"
+fi
+
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -Wshadow=com

Re: Add meson.build to version_stamp.pl

[Michael Paquier]

On Wed, Oct 05, 2022 at 11:20:50AM -0700, Andres Freund wrote:
> On 2022-10-05 11:36:36 +0100, Dagfinn Ilmari Mannsåker wrote:
>> sed_file("meson.build",
>>  qq{-e "/^project(/,/^)/ s/ version: '[0-9a-z.]*',/ version: 
>> '$fullversion',/"}
>> );
> 
> Yea, that looks nicer.

Oh.  That's nice..
--
Michael


signature.asc
Description: PGP signature

Re: shadow variables - pg15 edition

[David Rowley]

On Thu, 6 Oct 2022 at 11:50, David Rowley  wrote:
>
> On Thu, 6 Oct 2022 at 10:40, Andres Freund  wrote:
> > Your commit message said the last shadowed variable. But building with
> > -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed 
> > at
> > the end).  Looks like it "only" fixed it for src/, without optional
> > dependencies like gssapi and python.
>
> Well, that's embarrassing. You're right. I only fixed the ones I saw
> from running make in the base directory of the tree.  I'll set about
> fixing these nownow.

Here's a patch which (I think) fixes the ones I missed.

David
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index e64291e049..dd26d6ac29 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -232,8 +232,6 @@ blinsert(Relation index, Datum *values, bool *isnull,
 
if (metaData->nEnd > metaData->nStart)
{
-   Pagepage;
-
blkno = metaData->notFullPage[metaData->nStart];
Assert(blkno != InvalidBlockNumber);
 
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index de0b9a109c..67821cd25b 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -450,15 +450,15 @@ get_file_fdw_attribute_options(Oid relid)
for (attnum = 1; attnum <= natts; attnum++)
{
Form_pg_attribute attr = TupleDescAttr(tupleDesc, attnum - 1);
-   List   *options;
+   List   *column_options;
ListCell   *lc;
 
/* Skip dropped attributes. */
if (attr->attisdropped)
continue;
 
-   options = GetForeignColumnOptions(relid, attnum);
-   foreach(lc, options)
+   column_options = GetForeignColumnOptions(relid, attnum);
+   foreach(lc, column_options)
{
DefElem*def = (DefElem *) lfirst(lc);
 
@@ -480,7 +480,7 @@ get_file_fdw_attribute_options(Oid relid)
fncolumns = lappend(fncolumns, 
makeString(attname));
}
}
-   /* maybe in future handle other options here */
+   /* maybe in future handle other column options here */
}
}
 
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index 4713e6ea7a..897af244a4 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -128,15 +128,15 @@ typedef struct
 /* finalize a newly-constructed hstore */
 #define HS_FINALIZE(hsp_,count_,buf_,ptr_) 
\
do {
\
-   int buflen = (ptr_) - (buf_);   
\
+   int _buflen = (ptr_) - (buf_);  
\
if ((count_))   
\
ARRPTR(hsp_)[0].entry |= HENTRY_ISFIRST;
\
if ((count_) != HS_COUNT((hsp_)))   
\
{   
\
HS_SETCOUNT((hsp_),(count_));   
\
-   memmove(STRPTR(hsp_), (buf_), buflen);  
\
+   memmove(STRPTR(hsp_), (buf_), _buflen); 
\
}   
\
-   SET_VARSIZE((hsp_), CALCDATASIZE((count_), buflen));
\
+   SET_VARSIZE((hsp_), CALCDATASIZE((count_), _buflen));   
\
} while (0)
 
 /* ensure the varlena size of an existing hstore is correct */
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 09f37fb77a..9524765650 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -943,8 +943,6 @@ foreign_expr_walker(Node *node,
 */
if (agg->aggorder)
{
-   ListCell   *lc;
-
foreach(lc, agg->aggorder)
{
SortGroupClause *srt = 
(SortGroupClause *) lfirst(lc);
diff --git a/contrib/postgres_fdw/postgres_fdw.c 
b/contrib/postgres_fdw/postgres_fdw.c
index dd858aba03..8d013f5b1a 100644
--- a/con

Re: Move backup-related code to xlogbackup.c/.h

[Andres Freund]

Hi,

On 2022-10-05 15:22:01 +0530, Bharath Rupireddy wrote:
> +extern void WALInsertLockAcquire(void);
> +extern void WALInsertLockAcquireExclusive(void);
> +extern void WALInsertLockRelease(void);
> +extern void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);
> 
> Note that I had moved all WAL insert lock related functions to xlog.h
> despite xlogbackup.c using 2 of them. This is done to keep all the
> functions together.
> 
> Please review the attached v2 patch set.

I'm doubtful it's a good idea to expose these to outside of xlog.c - they are
very low level, and it's very easy to break stuff by using them wrongly. IMO,
if that's necessary, the split isn't right.

Greetings,

Andres Freund

Re: Move backup-related code to xlogbackup.c/.h

[Nathan Bossart]

On Wed, Oct 05, 2022 at 03:22:01PM +0530, Bharath Rupireddy wrote:
>> On Tue, Oct 04, 2022 at 03:54:20PM -0700, Nathan Bossart wrote:
>> > I would suggest moving this to a separate prerequisite patch that can be
>> > reviewed independently from the patches that simply move code to a
>> > different file.
> 
> I added the new functions in 0001 patch for ease of review.

Can we also replace the relevant code with calls to these functions in
0001?  That way, we can more easily review the changes you are making to
this code separately from the large patch that just moves the code.

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

Re: shadow variables - pg15 edition

[David Rowley]

On Thu, 6 Oct 2022 at 10:40, Andres Freund  wrote:
> Your commit message said the last shadowed variable. But building with
> -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
> the end).  Looks like it "only" fixed it for src/, without optional
> dependencies like gssapi and python.

Well, that's embarrassing. You're right. I only fixed the ones I saw
from running make in the base directory of the tree.  I'll set about
fixing these nownow.

David

Re: shadow variables - pg15 edition

[David Rowley]

On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera  wrote:
> A simpler idea might be to just remove the inner declaration, and have
> that block set the outer var.  There's no damage, since the block is
> going to end and not access the previous value anymore.
>
> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index aa1a3541fe..91a067859b 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -7506,7 +7506,7 @@ threadRun(void *arg)
> /* progress report is made by thread 0 for all threads */
> if (progress && thread->tid == 0)
> {
> -   pg_time_usec_t now = pg_time_now();
> +   now = pg_time_now();/* not lazy; clobbers outer 
> value */

I didn't want to do it that way because all this code is in a while
loop and the outer "now" will be reused after it's set by the code
above.  It's not really immediately obvious to me what repercussions
that would have, but it didn't seem worth taking any risks.

David

pg_statsinfo - PG15 support?

[Greg Nancarrow]

Hi,

To the developer(s) who work(s) on pg_statsinfo, are there plans to
support PG15 and when might pg_statsinfo v15 source be released?
I can see that for v14 of pg_statsinfo there is an incompatibility
with the PG15 autovacuum log (i.e. in PG15 some existing autovacuum
log fields have been removed and some new fields have been added). I
also noticed that PG15 changes how shared libraries must request
additional shared memory during initialization and pg_statsinfo source
code would need updating for this.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: [PATCH] Expand character set for ltree labels

[Garen Torikian]

After digging into it, you are completely correct. I had to do a bit more
reading to understand the relationships between UTF-8 and wchar, but
ultimately the existing locale support works for my use case.

Therefore I have updated the patch with three much smaller changes:

* Support for `-` in addition to `_`
* Expanding the limit to 512 chars (from the existing 256); again it's not
uncommon for non-English strings to be much longer
* Fixed the documentation to expand on what the ltree label's relationship
to the DB locale is

Thank you,
Garen

On Wed, Oct 5, 2022 at 3:56 PM Tom Lane  wrote:

> Garen Torikian  writes:
> >> Perhaps the docs are a bit unclear about that, but it's not
> >> restricted to ASCII alphanumerics.  AFAICS the code will accept
> >> whatever iswalpha() and iswdigit() will accept in the database's
> >> default locale.
>
> > Sorry but I don't think that is correct. Here is the single
> > definition check of what constitutes a valid character:
> >
> https://github.com/postgres/postgres/blob/c3315a7da57be720222b119385ed0f7ad7c15268/contrib/ltree/ltree.h#L129
>
> > As you can see, there are no `is_*` calls at all.
>
> Did you chase down what t_isalpha and t_isdigit do?
>
> regards, tom lane
>


0002-Expand-character-set-for-ltree-labels.patch
Description: Binary data

Re: shadow variables - pg15 edition

[Andres Freund]

Hi,

On 2022-10-06 10:21:41 +1300, David Rowley wrote:
> Also pushed.  (Thanks for saving me on that one.)

Your commit message said the last shadowed variable. But building with
-Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
the end).  Looks like it "only" fixed it for src/, without optional
dependencies like gssapi and python.

I think we should add -Wshadow=compatible-local to our sets of warning flags
after fixing those.


[237/1827 42  12%] Compiling C object 
src/interfaces/libpq/libpq.a.p/fe-secure-gssapi.c.o
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c: 
In function ‘pg_GSS_write’:
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:138:41:
 warning: declaration of ‘ret’ shadows a previous local 
[-Wshadow=compatible-local]
  138 | ssize_t ret;
  | ^~~
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:92:25:
 note: shadowed declaration is here
   92 | ssize_t ret = -1;
  | ^~~


[1283/1827 42  70%] Compiling C object 
src/pl/plpython/plpython3.so.p/plpy_cursorobject.c.o
In file included from 
../../../../home/andres/src/postgresql/src/include/postgres.h:48,
 from 
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c: In 
function ‘PLy_cursor_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: 
warning: declaration of ‘_save_exception_stack’ shadows a previous local 
[-Wshadow=compatible-local]
  325 | sigjmp_buf *_save_exception_stack##__VA_ARGS__ = 
PG_exception_stack; \
  | ^
...

[1289/1827 42  70%] Compiling C object 
src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c: In function 
‘PLy_exec_trigger’:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:378:46: 
warning: declaration of ‘tdata’ shadows a previous local 
[-Wshadow=compatible-local]
  378 | TriggerData *tdata = (TriggerData *) 
fcinfo->context;
  |  ^
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:310:22: 
note: shadowed declaration is here
  310 | TriggerData *tdata;
  |  ^

[1291/1827 42  70%] Compiling C object 
src/pl/plpython/plpython3.so.p/plpy_spi.c.o
In file included from 
../../../../home/andres/src/postgresql/src/include/postgres.h:48,
 from 
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c: In function 
‘PLy_spi_execute_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: 
warning: declaration of ‘_save_exception_stack’ shadows a previous local 
[-Wshadow=compatible-local]
  325 | sigjmp_buf *_save_exception_stack##__VA_ARGS__ = 
PG_exception_stack; \
  | ^


[1344/1827 42  73%] Compiling C object contrib/bloom/bloom.so.p/blinsert.c.o
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c: In function 
‘blinsert’:
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:235:33: 
warning: declaration of ‘page’ shadows a previous local 
[-Wshadow=compatible-local]
  235 | Pagepage;
  | ^~~~
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:210:25: note: 
shadowed declaration is here
  210 | Pagepage,
  | ^~~~

[1415/1827 42  77%] Compiling C object 
contrib/file_fdw/file_fdw.so.p/file_fdw.c.o
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c: In function 
‘get_file_fdw_attribute_options’:
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:453:29: 
warning: declaration of ‘options’ shadows a previous local 
[-Wshadow=compatible-local]
  453 | List   *options;
  | ^~~
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:443:21: 
note: shadowed declaration is here
  443 | List   *options = NIL;
  | ^~~

[1441/1827 42  78%] Compiling C object contrib/hstore/hstore.so.p/hstore_io.c.o
In file included from 
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:12:
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c: In function 
‘hstorePairs’:
../../../../home/andres/src/postgresql/contrib/hstore/hstore.h:131:21: warning: 
declaration of ‘buflen’ shadows a parameter [-Wshadow=compatible-local]
  131 | int buflen = (ptr_) - (buf_);   
   

Re: future of serial and identity columns

[Bruce Momjian]

On Tue, Oct  4, 2022 at 09:41:19AM +0200, Peter Eisentraut wrote:
> In PostgreSQL 10, we added identity columns, as an alternative to serial
> columns (since 6.something).  They mostly work the same.  Identity columns
> are SQL-conforming, have some more features (e.g., overriding clause), and
> are a bit more robust in schema management.  Some of that was described in
> [0].  AFAICT, there have been no complaints since that identity columns lack
> features or are somehow a regression over serial columns.

FYI, SERIAL came from Informix syntax, and it was already a macro, so
making it a different macro seems fine.  ;-)

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: shadow variables - pg15 edition

[David Rowley]

On Thu, 6 Oct 2022 at 03:19, Tom Lane  wrote:
>
> David Rowley  writes:
> > I've attached a draft patch for a method I was considering to fix the
> > warnings we're getting from the nested PG_TRY() statement in
> > utility.c.
>
> +1

Pushed.

> > The only warning remaining after applying the attached is the "now"
> > warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> > which translates to "right now" in the part of Scotland that I'm from.
> > I also considered "nownow", which is used in South Africa [1].
> > Anyway, I'm not really being serious, but I didn't come up with
> > anything better than "now2".
>
> Yeah, "now2" seems as reasonable as anything.

Also pushed.  (Thanks for saving me on that one.)

David

Re: meson PGXS compatibility

[Andres Freund]

Hi,

On 2022-10-05 16:58:46 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > My understanding, from that commit message, was that the issue originates in
> > apple's ranlib setting the timestamp to its components but only queries / 
> > sets
> > it using second granularity. I verified that apple's ranlib and ar these 
> > days
> > just set the current time, at a high granularity, as the mtime.  Whether or
> > not make then hides the problem seems not that relevant if the source of the
> > problem is gone, no?
> 
> Well, (a) it seemed to happen in only some circumstances even back then,
> so maybe your testing didn't catch it; and (b) even assuming that Apple
> has fixed it in recent releases, there may still be people using older,
> un-fixed versions.  Why's it such a problem to keep the "touch" step?

It isn't! That's why I said that I was on the fence about removing the touch
in my first email and then that I'd leave the touch there and just
s/ranlib/ar/ in my reply to you.

Greetings,

Andres Freund

Re: An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication

[Bruce Momjian]

On Sat, Oct  1, 2022 at 06:59:26AM +0530, Bharath Rupireddy wrote:
> > I have always felt this has to be done at the server level, meaning when
> > a synchronous_standby_names replica is not responding after a certain
> > timeout, the administrator must be notified by calling a shell command
> > defined in a GUC and all sessions will ignore the replica.  This gives a
 
> > much more predictable and useful behavior than the one in the patch ---
> > we have discussed this approach many times on the email lists.
> 
> IIUC, each walsender serving a sync standby will determine that the
> sync standby isn't responding for a configurable amount of time (less
> than wal_sender_timeout) and calls shell command to notify the admin
> if there are any backends waiting for sync replication in
> SyncRepWaitForLSN(). The shell command then provides the unresponsive
> sync standby name at the bare minimum for the admin to ignore it as
> sync standby/remove it from synchronous_standby_names to continue
> further. This still requires manual intervention which is a problem if
> running postgres server instances at scale. Also, having a new shell

As I highlighted above, by default you notify the administrator that a
sychronous replica is not responding and then ignore it.  If it becomes
responsive again, you notify the administrator again and add it back as
a sychronous replica.

> command in any form may pose security risks. I'm not sure at this
> point how this new timeout is going to work alongside
> wal_sender_timeout.

We have archive_command, so I don't see a problem with another shell
command.

> I'm thinking about the possible options that an admin has to get out
> of this situation:
> 1) Removing the standby from synchronous_standby_names.

Yes, see above.  We might need a read-only GUC that reports which
sychronous replicas are active.  As you can see, there is a lot of API
design required here, but this is the most effective approach.

> 2) Fixing the sync standby, by restarting or restoring the lost part
> (such as network or some other).
> 
> (1) is something that postgres can help admins get out of the problem
> easily and automatically without any intervention. (2) is something
> postgres can't do much about.
> 
> How about we let postgres automatically remove an unresponsive (for a
> pre-configured time) sync standby from synchronous_standby_names and
> inform the user (via log message and via new walsender property and
> pg_stat_replication for monitoring purposes)? The users can then
> detect such standbys and later try to bring them back to the sync
> standbys group or do other things. I believe that a production level
> postgres HA with sync standbys will have monitoring to detect the
> replication lag, failover decision etc via monitoring
> pg_stat_replication. With this approach, a bit more monitoring is
> needed. This solution requires less or no manual intervention and
> scales well. Please note that I haven't studied the possibilities of
> implementing it yet.
> 
> Thoughts?

Yes, see above.

> > Once we have that, we can consider removing the cancel ability while
> > waiting for synchronous replicas (since we have the timeout) or make it
> > optional.  We can also consider how do notify the administrator during
> > query cancel (if we allow it), backend abrupt exit/crash, and
> 
> Yeah. If we have the
> timeout-and-auto-removal-of-standby-from-sync-standbys-list solution,
> the users can then choose to disable processing query cancels/proc
> dies while waiting for sync replication in SyncRepWaitForLSN().

Yes.  We might also change things so a query cancel that happens during 
sychronous replica waiting can only be done by an administrator, not the
session owner.  Again, lots of design needed here.

> > if we
> > should allow users to specify a retry interval to resynchronize the
> > synchronous replicas.
> 
> This is another interesting thing to consider if we were to make the
> auto-removed (by the above approach) standby a sync standby again
> without manual intervention.

Yes, see above.  You are addressing the right questions here.  :-)

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: meson PGXS compatibility

[Tom Lane]

Andres Freund  writes:
> My understanding, from that commit message, was that the issue originates in
> apple's ranlib setting the timestamp to its components but only queries / sets
> it using second granularity. I verified that apple's ranlib and ar these days
> just set the current time, at a high granularity, as the mtime.  Whether or
> not make then hides the problem seems not that relevant if the source of the
> problem is gone, no?

Well, (a) it seemed to happen in only some circumstances even back then,
so maybe your testing didn't catch it; and (b) even assuming that Apple
has fixed it in recent releases, there may still be people using older,
un-fixed versions.  Why's it such a problem to keep the "touch" step?

regards, tom lane

Re: meson PGXS compatibility

[Andres Freund]

Hi,

On 2022-10-05 16:20:22 -0400, Tom Lane wrote:
> Andres Freund  writes:
> >   On macOS we ran ranlib after installing a static library. This was added a
> >   long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
> >   macOS versions.
> 
> I agree that shouldn't be necessary anymore (and if it is, we'll find
> out soon enough).

Cool.


> >   I'm on the fence about removing the "touch $@" from the rule building 
> > static
> >   libs. That was added because of macos's ranlib not setting fine-grained
> >   timestamps. On a modern mac ar and ranlib are the same binary, and maybe
> >   that means that ar has the same issue? Both do set fine-grained
> >   timestamps:
> 
> Please see the commit message for 826eff57c4c: the issue seems to arise
> only with specific combinations of software, in particular with non-Apple
> versions of "make" (although maybe later Apple builds have fixed make's
> failure to read sub-second timestamps?).

My understanding, from that commit message, was that the issue originates in
apple's ranlib setting the timestamp to its components but only queries / sets
it using second granularity. I verified that apple's ranlib and ar these days
just set the current time, at a high granularity, as the mtime.  Whether or
not make then hides the problem seems not that relevant if the source of the
problem is gone, no?


> That's a relatively recent hack, and I'm very hesitant to conclude that we
> don't need it anymore just because you failed to reproduce an issue locally.

Yea, that's why I was hesitant as well. I'll reformulate the comment to
reference ar instead of ranlib instead.


> It very possibly isn't a problem in a meson build, though, depending on how
> much meson depends on file timestamps.

Most of the timestamp sensitive stuff is dealt with by ninja, rather than
meson. ninja does take timestamps into account when determining what to
rebuild - although I suspect this specific problem wouldn't occur even with a
problematic ar/ranlib version, because the relevant timestamps will be on the
.c (etc) files, rather than the .a. Ninja has the whole dependency graph, so
it knows what dependencies it has to rebuild, without needing to check
timestamps of intermediary objects.

Ninja does support build rules where it checks the timestamps of build outputs
to see if targets depending on those build outputs have to be rebuilt, or not,
because the target didn't change. But the relevant option ("restat") isn't set
for compiler / linker invocations in the build.ninja meson generates.

Restat is however set for the "custom_command"s we use to generate all kinds
of sources. Sometimes that leads to the set of build steps shrinking
rapidly. E.g. a touch src/include/catalog/pg_namespace.dat starts leads to
ninja considering 1135 targets out of date, but as genbki.pl doesn't end up
changing any files, it's done immediately after that...

[1/1135 1   0%] Generating src/include/catalog/generated_catalog_headers with a 
custom command

Greetings,

Andres Freund

Re: meson: Add support for building with precompiled headers

[Andres Freund]

Hi,

On 2022-10-05 16:21:55 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2022-10-05 16:09:14 -0400, Tom Lane wrote:
> >> Color me confused, but how does it work to #define that from the command
> >> line if it can't be overridden from within the program?
> 
> > If specified on the commandline it's also used when generating the 
> > precompiled
> > header - of course that's not possible when it's just #define'd in some .c
> > file.
> 
> Ah, so there's a separate cache of precompiled headers for each set of
> compiler command-line arguments?  Got it.

Worse, it builds the precompiled header for each "target" (static/shared lib,
executable), right now. Hence I've only added them for targets that have
multiple .c files. I've been planning to submit an improvement to meson that
does what you propose, it'd not be hard, but before it's actually usable, it
didn't seem worth investing time in that.

Greetings,

Andres Freund

Re: meson: Add support for building with precompiled headers

[Tom Lane]

Andres Freund  writes:
> On 2022-10-05 16:09:14 -0400, Tom Lane wrote:
>> Color me confused, but how does it work to #define that from the command
>> line if it can't be overridden from within the program?

> If specified on the commandline it's also used when generating the precompiled
> header - of course that's not possible when it's just #define'd in some .c
> file.

Ah, so there's a separate cache of precompiled headers for each set of
compiler command-line arguments?  Got it.

regards, tom lane

Re: meson PGXS compatibility

[Tom Lane]

Andres Freund  writes:
>   On macOS we ran ranlib after installing a static library. This was added a
>   long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
>   macOS versions.

I agree that shouldn't be necessary anymore (and if it is, we'll find
out soon enough).

>   I'm on the fence about removing the "touch $@" from the rule building static
>   libs. That was added because of macos's ranlib not setting fine-grained
>   timestamps. On a modern mac ar and ranlib are the same binary, and maybe
>   that means that ar has the same issue? Both do set fine-grained
>   timestamps:

Please see the commit message for 826eff57c4c: the issue seems to arise
only with specific combinations of software, in particular with non-Apple
versions of "make" (although maybe later Apple builds have fixed make's
failure to read sub-second timestamps?).  That's a relatively recent hack,
and I'm very hesitant to conclude that we don't need it anymore just
because you failed to reproduce an issue locally.  It very possibly isn't
a problem in a meson build, though, depending on how much meson depends on
file timestamps.

regards, tom lane

Re: meson: Add support for building with precompiled headers

[Andres Freund]

Hi,

On 2022-10-05 16:09:14 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > When using precompiled headers we cannot override macros in system headers
> > from within .c files, as headers are already processed before the #define in
> > the C file is reached.
> 
> > A few files #define FD_SETSIZE 1024 on windows, as the default is only 64. I
> > am hesitant to change FD_SETSIZE globally on windows, due to
> > src/backend/port/win32/socket.c using it to size on-stack arrays. Instead 
> > add
> > -DFD_SETSIZE=1024 when building the specific targets needing it.
> 
> Color me confused, but how does it work to #define that from the command
> line if it can't be overridden from within the program?

If specified on the commandline it's also used when generating the precompiled
header - of course that's not possible when it's just #define'd in some .c
file.

Greetings,

Andres Freund

Re: meson: Add support for building with precompiled headers

[Tom Lane]

Andres Freund  writes:
> When using precompiled headers we cannot override macros in system headers
> from within .c files, as headers are already processed before the #define in
> the C file is reached.

> A few files #define FD_SETSIZE 1024 on windows, as the default is only 64. I
> am hesitant to change FD_SETSIZE globally on windows, due to
> src/backend/port/win32/socket.c using it to size on-stack arrays. Instead add
> -DFD_SETSIZE=1024 when building the specific targets needing it.

Color me confused, but how does it work to #define that from the command
line if it can't be overridden from within the program?

regards, tom lane

meson PGXS compatibility

[Andres Freund]

Hi,

As the meson support stands right now, one cannot easily build an extension
against a postgres built with meson. As discussed at the 2022 unconference
[1], one way to make that easier for extensions is for meson to generate a
complete enough Makefile.global for pgxs.mk to work.

This is a series of patches towards that goal. The first four simplify some
aspects of Makefile.global, and then the fifth generates Makefile.global etc.


0001: autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C

  Right now emit the cflags to build the CRC objects into architecture specific
  variables. That doesn't make a whole lot of sense to me - we're never going to
  target x86 and arm at the same time, so they don't need to be separate
  variables.

  It might be better to instead continue to have CFLAGS_SSE42 /
  CFLAGS_ARMV8_CRC32C be computed by PGAC_ARMV8_CRC32C_INTRINSICS /
  PGAC_SSE42_CRC32_INTRINSICS and then set CFLAGS_CRC based on those. But it
  seems unlikely that we'd need other sets of flags for those two architectures
  at the same time.

  The separate flags could be supported by the meson build instead, it'd just
  add unneccessary complexity.


0002: autoconf: Rely on ar supporting index creation

  With meson we don't require ranlib. But as it is set in Makefile.global and
  used by several platforms, we'd need to detect it.

  FreeBSD, NetBSD, OpenBSD, the only platforms were we didn't use AROPT=crs,
  all have supported the 's' option for a long time.

  On macOS we ran ranlib after installing a static library. This was added a
  long time ago, in 58ad65ec2def. I cannot reproduce an issue in more recent
  macOS versions.

  I'm on the fence about removing the "touch $@" from the rule building static
  libs. That was added because of macos's ranlib not setting fine-grained
  timestamps. On a modern mac ar and ranlib are the same binary, and maybe
  that means that ar has the same issue? Both do set fine-grained
  timestamps:

  cc ../test.c -o test.o -c
  rm -f test.a; ar csr test.a test.o ; ranlib test.a; python3 -c "import 
os;print(os.stat('test.a').st_mtime_ns)"
  1664999109090448534

  But I don't know how far back that goes. We could just reformulate the
  comment to mention ar instead of ranlib.


  Tom, CCing you due to 58ad65ec2def and 826eff57c4c.


0003: aix: Build SUBSYS.o using $(CC) -r instead of $(LD) -r

  This is the only direct use of $(LD), and xlc -r and gcc -r end up with the
  same set of symbols and similar performance (noise is high, so hard to say if
  equivalent).

  Now that $(LD) isn't needed anymore, remove it from src/Makefile.global

  While at it, add a comment why -r is used.


0004: solaris: Check for -Wl,-E directly instead of checking for gnu LD

  This allows us to get rid of the nontrivial detection of with_gnu_ld,
  simplifying meson PGXS compatibility. It's also nice to delete libtool.m4.

  I don't like the SOLARIS_EXPORT_DYNAMIC variable I invented. If somebody has
  a better idea...


0005: meson: Add PGXS compatibility

  The actual meson PGXS compatibility. Plenty more replacements to do, but
  suffices to build common extensions on a few platforms.

  What level of completeness do we want to require here?


  A few replacements worth thinking about:

  - autodepend - I'm inclined to set it to true when using a gcc like
compiler. I think extension authors won't be happy if suddenly their
extensions don't rebuild reliably anymore. An --enable-depend like
setting doesn't make sense for meson, so we don't have anything to source it
from.
  - {LDAP,UUID,ICU}_{LIBS,CFLAGS} - might some extension need them?


  For some others I think it's ok to not have replacement. Would be good for
  somebody to check my thinking though:

  - LIBOBJS, PG_CRC32C_OBJS, TAS: Not needed because we don't build
the server / PLs with the generated makefile
  - ZIC: only needed to build tzdata as part of server build
  - MSGFMT et al: translation doesn't appear to be supported by pgxs, correct?
  - XMLLINT et al: docs don't seem to be supported by pgxs
  - GENHTML et al: supporting coverage for pgxs-in-meson build doesn't seem 
worth it
  - WINDRES: I don't think extensions are bothering to generate rc files on 
windows


My colleague Bilal has set up testing and verified that a few extensions build
with the pgxs compatibility layer, on linux at last. Currently pg_qualstats,
pg_cron, hypopg, orafce, postgis, pg_partman work. He also tested pgbouncer,
but for him that failed both with autoconf and meson generated pgxs.

I wonder if and where we could have something like this tested continually?

Greetings,

Andres Freund

[1] 
https://wiki.postgresql.org/wiki/PgCon_2022_Developer_Unconference#Meson_new_build_system_proposal
>From 6e24c54141c3063db6d4b2e6e4d43d1cfa563deb Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 5 Oct 2022 12:24:14 -0700
Subject: [PATCH v2 1/5] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C

Until now we emit the cflags to bu

Re: [PATCH] Expand character set for ltree labels

[Tom Lane]

Garen Torikian  writes:
>> Perhaps the docs are a bit unclear about that, but it's not
>> restricted to ASCII alphanumerics.  AFAICS the code will accept
>> whatever iswalpha() and iswdigit() will accept in the database's
>> default locale.

> Sorry but I don't think that is correct. Here is the single
> definition check of what constitutes a valid character:
> https://github.com/postgres/postgres/blob/c3315a7da57be720222b119385ed0f7ad7c15268/contrib/ltree/ltree.h#L129

> As you can see, there are no `is_*` calls at all.

Did you chase down what t_isalpha and t_isdigit do?

regards, tom lane

ts_locale.c: why no t_isalnum() test?

[Tom Lane]

I happened to wonder why various places are testing things like

#define ISWORDCHR(c)(t_isalpha(c) || t_isdigit(c))

rather than using an isalnum-equivalent test.  The direct answer
is that ts_locale.c/.h provides no such test function, which
apparently is because there's not a lot of potential callers in
the core code.  However, both pg_trgm and ltree could benefit
from adding one.

There's no semantic hazard here: the documentation I consulted
is all pretty explicit that is[w]alnum is true exactly when
either is[w]alpha or is[w]digit are.  For example, POSIX saith

The iswalpha() and iswalpha_l() functions shall test whether wc is a
wide-character code representing a character of class alpha in the
current locale, or in the locale represented by locale, respectively;
see XBD Locale.

The iswdigit() and iswdigit_l() functions shall test whether wc is a
wide-character code representing a character of class digit in the
current locale, or in the locale represented by locale, respectively;
see XBD Locale.

The iswalnum() and iswalnum_l() functions shall test whether wc is a
wide-character code representing a character of class alpha or digit
in the current locale, or in the locale represented by locale,
respectively; see XBD Locale.

While I didn't try to actually measure it, these functions don't
look remarkably cheap.  Doing char2wchar() twice when we only need
to do it once seems silly, and the libc functions themselves are
probably none too cheap for multibyte characters either.

Hence, I propose the attached.  I got rid of some places that were
unnecessarily checking pg_mblen before applying t_iseq(), too.

regards, tom lane

diff --git a/contrib/ltree/lquery_op.c b/contrib/ltree/lquery_op.c
index d580339283..a6466f575f 100644
--- a/contrib/ltree/lquery_op.c
+++ b/contrib/ltree/lquery_op.c
@@ -25,17 +25,16 @@ static char *
 getlexeme(char *start, char *end, int *len)
 {
 	char	   *ptr;
-	int			charlen;
 
-	while (start < end && (charlen = pg_mblen(start)) == 1 && t_iseq(start, '_'))
-		start += charlen;
+	while (start < end && t_iseq(start, '_'))
+		start += pg_mblen(start);
 
 	ptr = start;
 	if (ptr >= end)
 		return NULL;
 
-	while (ptr < end && !((charlen = pg_mblen(ptr)) == 1 && t_iseq(ptr, '_')))
-		ptr += charlen;
+	while (ptr < end && !t_iseq(ptr, '_'))
+		ptr += pg_mblen(ptr);
 
 	*len = ptr - start;
 	return start;
diff --git a/contrib/ltree/ltree.h b/contrib/ltree/ltree.h
index 40aed0ca0c..2a80a02495 100644
--- a/contrib/ltree/ltree.h
+++ b/contrib/ltree/ltree.h
@@ -126,7 +126,7 @@ typedef struct
 
 #define LQUERY_HASNOT		0x01
 
-#define ISALNUM(x)	( t_isalpha(x) || t_isdigit(x)	|| ( pg_mblen(x) == 1 && t_iseq((x), '_') ) )
+#define ISALNUM(x)	( t_isalnum(x) || t_iseq(x, '_') )
 
 /* full text query */
 
diff --git a/contrib/ltree/ltxtquery_io.c b/contrib/ltree/ltxtquery_io.c
index 3eca5cb8ff..8ab0ce8e52 100644
--- a/contrib/ltree/ltxtquery_io.c
+++ b/contrib/ltree/ltxtquery_io.c
@@ -64,13 +64,13 @@ gettoken_query(QPRS_STATE *state, int32 *val, int32 *lenval, char **strval, uint
 		switch (state->state)
 		{
 			case WAITOPERAND:
-if (charlen == 1 && t_iseq(state->buf, '!'))
+if (t_iseq(state->buf, '!'))
 {
 	(state->buf)++;
 	*val = (int32) '!';
 	return OPR;
 }
-else if (charlen == 1 && t_iseq(state->buf, '('))
+else if (t_iseq(state->buf, '('))
 {
 	state->count++;
 	(state->buf)++;
@@ -97,11 +97,11 @@ gettoken_query(QPRS_STATE *state, int32 *val, int32 *lenval, char **strval, uint
  errmsg("modifiers syntax error")));
 	*lenval += charlen;
 }
-else if (charlen == 1 && t_iseq(state->buf, '%'))
+else if (t_iseq(state->buf, '%'))
 	*flag |= LVAR_SUBLEXEME;
-else if (charlen == 1 && t_iseq(state->buf, '@'))
+else if (t_iseq(state->buf, '@'))
 	*flag |= LVAR_INCASE;
-else if (charlen == 1 && t_iseq(state->buf, '*'))
+else if (t_iseq(state->buf, '*'))
 	*flag |= LVAR_ANYEND;
 else
 {
@@ -110,14 +110,14 @@ gettoken_query(QPRS_STATE *state, int32 *val, int32 *lenval, char **strval, uint
 }
 break;
 			case WAITOPERATOR:
-if (charlen == 1 && (t_iseq(state->buf, '&') || t_iseq(state->buf, '|')))
+if (t_iseq(state->buf, '&') || t_iseq(state->buf, '|'))
 {
 	state->state = WAITOPERAND;
 	*val = (int32) *(state->buf);
 	(state->buf)++;
 	return OPR;
 }
-else if (charlen == 1 && t_iseq(state->buf, ')'))
+else if (t_iseq(state->buf, ')'))
 {
 	(state->buf)++;
 	state->count--;
@@ -125,7 +125,7 @@ gettoken_query(QPRS_STATE *state, int32 *val, int32 *lenval, char **strval, uint
 }
 else if (*(state->buf) == '\0')
 	return (state->count) ? ERR : END;
-else if (charlen == 1 && !t_iseq(state->buf, ' '))
+else if (!t_iseq(state->buf, ' '))
 	return ERR;
 break;
 			default:
diff --git a/contrib/pg_trgm/trgm.h b/contrib/pg_trgm/trgm.h
index 40

Re: [PATCH] Expand character set for ltree labels

[Garen Torikian]

Hi Tom,

> Perhaps the docs are a bit unclear about that, but it's not
> restricted to ASCII alphanumerics.  AFAICS the code will accept
> whatever iswalpha() and iswdigit() will accept in the database's
> default locale.

Sorry but I don't think that is correct. Here is the single
definition check of what constitutes a valid character:
https://github.com/postgres/postgres/blob/c3315a7da57be720222b119385ed0f7ad7c15268/contrib/ltree/ltree.h#L129

As you can see, there are no `is_*` calls at all. Where in this contrib
package do you see `iswalpha`? Perhaps I missed it.

> That seems really pretty random.

Ok. I am trying to avoid a situation where other users may wish to use
other delimiters other than `-`, due to its commonplace presence in words
(eg., compound ones).

On Wed, Oct 5, 2022 at 2:59 PM Tom Lane  wrote:

> Garen Torikian  writes:
> > I am submitting a patch to expand the label requirements for ltree.
>
> > The current format is restricted to alphanumeric characters, plus _.
> > Unfortunately, for non-English labels, this set is insufficient.
>
> Hm?  Perhaps the docs are a bit unclear about that, but it's not
> restricted to ASCII alphanumerics.  AFAICS the code will accept
> whatever iswalpha() and iswdigit() will accept in the database's
> default locale.  There's certainly work that could/should be done
> to allow use of not-so-default locales, but that's not specific
> to ltree.  I'm not sure that doing an application-side encoding
> is attractive compared to just using that ability directly.
>
> If you do want to do application-side encoding, I'm unsure why
> punycode would be the choice anyway, as opposed to something
> that can fit in the existing restrictions.
>
> > On top of this, I added support for two more characters: # and ;, which
> are
> > used for HTML entities.
>
> That seems really pretty random.
>
> regards, tom lane
>

meson: Add support for building with precompiled headers

[Andres Freund]

Hi,

This is a patch split off from the initial meson thread [1] as it's
functionally largely independent (as suggested in [2]).

Using precompiled headers substantially speeds up building for windows, due to
the vast amount of headers included via windows.h. A cross build from
linux targetting mingw goes from

994.11user 136.43system 0:31.58elapsed 3579%CPU
to
422.41user 89.05system 0:14.35elapsed 3562%CPU

The wins on windows are similar-ish (but I don't have a system at hand just
now for actual numbers). Targetting other operating systems the wins are far
smaller (tested linux, macOS, FreeBSD).

This is particularly interesting for cfbot, which spends a lot of time
building on windows.  It also makes developing on windows less painful as the
gains are bigger when compiling incrementally, because the precompiled headers
don't typically have to be rebuilt.


As a prerequisite this requires changing the way FD_SETSIZE is defined when
targetting windows.

When using precompiled headers we cannot override macros in system headers
from within .c files, as headers are already processed before the #define in
the C file is reached.

A few files #define FD_SETSIZE 1024 on windows, as the default is only 64. I
am hesitant to change FD_SETSIZE globally on windows, due to
src/backend/port/win32/socket.c using it to size on-stack arrays. Instead add
-DFD_SETSIZE=1024 when building the specific targets needing it.

We likely should move away from using select() in those places, but that's a
larger change.


Michael, CCing you wrt the second patch, as Thomas noticed [3] that you were
looking at where to define FD_SETSIZE.

Greetings,

Andres Freund

[1] 
https://www.postgresql.org/message-id/20211012083721.hvixq4pnh2pixr3j%40alap3.anarazel.de
[2] 
https://www.postgresql.org/message-id/e0c44fb2-8b66-a4b9-b274-7ed3a1a0ab74%40enterprisedb.com
[3] 
https://www.postgresql.org/message-id/CA+hUKG+50eOUbN++ocDc0Qnp9Pvmou23DSXu=za6fepocft...@mail.gmail.com
[4] https://www.postgresql.org/message-id/20190826054000.GE7005%40paquier.xyz
>From 7c0a0cff9b366367b8cfd51302f944de83579dca Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 25 Sep 2022 12:07:29 -0700
Subject: [PATCH v2 1/2] windows: adjust FD_SETSIZE via commandline define

When using precompiled headers we cannot override macros in system headers
from within .c files, as headers are already processed before the #define in
the C file is reached.

A few files #define FD_SETSIZE 1024 on windows, as the default is only 64. I
am hesitant to change FD_SETSIZE globally on windows, due to
src/backend/port/win32/socket.c using it to size on-stack arrays. Instead add
-DFD_SETSIZE=1024 when building the specific targets needing it.

We likely should move away from using select() in those places, but that's a
larger change.

Reviewed-by: Thomas Munro 
Reviewed-by: Justin Pryzby 
Discussion: https://postgr.es/m/CA+hUKG+50eOUbN++ocDc0Qnp9Pvmou23DSXu=za6fepocft...@mail.gmail.com
Discussion: https://postgr.es/m/20190826054000.GE7005%40paquier.xyz
---
 src/fe_utils/Makefile|  4 
 src/fe_utils/meson.build |  1 +
 src/fe_utils/parallel_slot.c |  4 ++--
 src/bin/pgbench/Makefile |  2 ++
 src/bin/pgbench/meson.build  |  1 +
 src/bin/pgbench/pgbench.c|  4 ++--
 src/tools/msvc/Mkvcbuild.pm  | 27 +++
 7 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index 44bc7a1215d..40d4b1d8698 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -34,6 +34,10 @@ OBJS = \
 	simple_list.o \
 	string_utils.o
 
+ifeq ($(PORTNAME), win32)
+override CFLAGS += -DFD_SETSIZE=1024
+endif
+
 all: libpgfeutils.a
 
 libpgfeutils.a: $(OBJS)
diff --git a/src/fe_utils/meson.build b/src/fe_utils/meson.build
index b6bf8e1ca21..3e226c260ac 100644
--- a/src/fe_utils/meson.build
+++ b/src/fe_utils/meson.build
@@ -24,6 +24,7 @@ fe_utils_sources += psqlscan
 fe_utils = static_library('libpgfeutils',
   fe_utils_sources + generated_headers,
   include_directories: [postgres_inc, libpq_inc],
+  c_args: host_system == 'windows' ? ['-DFD_SETSIZE=1024'] : [],
   dependencies: frontend_common_code,
   kwargs: default_lib_args,
 )
diff --git a/src/fe_utils/parallel_slot.c b/src/fe_utils/parallel_slot.c
index 2be2903c9c6..767256757f2 100644
--- a/src/fe_utils/parallel_slot.c
+++ b/src/fe_utils/parallel_slot.c
@@ -12,8 +12,8 @@
  *-
  */
 
-#ifdef WIN32
-#define FD_SETSIZE 1024			/* must set before winsock2.h is included */
+#if defined(WIN32) && FD_SETSIZE < 1024
+#error FD_SETSIZE needs to have been increased
 #endif
 
 #include "postgres_fe.h"
diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index 6647c9fe97d..68e9f03e79f 100644
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -18,6 +18,8 @@ LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
 
 ifneq ($(PORTNAME), win32)
 override C

Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records

[Nathan Bossart]

On Wed, Oct 05, 2022 at 11:30:22AM -0400, Tom Lane wrote:
> One other point to discuss: should we consider back-patching?  I've
> got mixed feelings about that myself.  I don't think that cases where
> this helps significantly are at all mainstream, so I'm kind of leaning
> to "patch HEAD only".

+1.  It can always be back-patched in the future if there are additional
reports.

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

Re: [PATCH] Expand character set for ltree labels

[Tom Lane]

Garen Torikian  writes:
> I am submitting a patch to expand the label requirements for ltree.

> The current format is restricted to alphanumeric characters, plus _.
> Unfortunately, for non-English labels, this set is insufficient.

Hm?  Perhaps the docs are a bit unclear about that, but it's not
restricted to ASCII alphanumerics.  AFAICS the code will accept
whatever iswalpha() and iswdigit() will accept in the database's
default locale.  There's certainly work that could/should be done
to allow use of not-so-default locales, but that's not specific
to ltree.  I'm not sure that doing an application-side encoding
is attractive compared to just using that ability directly.

If you do want to do application-side encoding, I'm unsure why
punycode would be the choice anyway, as opposed to something
that can fit in the existing restrictions.

> On top of this, I added support for two more characters: # and ;, which are
> used for HTML entities.

That seems really pretty random.

regards, tom lane

Re: archive modules

[Nathan Bossart]

On Wed, Oct 05, 2022 at 07:55:58PM +0200, Peter Eisentraut wrote:
> Leaving archive_command empty is an intentional configuration choice.
> 
> What we are talking about here is, arguably, a misconfiguration, so it
> should result in an error.

Okay.  What do you think about something like the attached?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d750290f13..bb4d985f35 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3597,9 +3597,11 @@ include_dir 'conf.d'


 This parameter can only be set in the postgresql.conf
-file or on the server command line.  It is ignored unless
+file or on the server command line.  It is only used if
 archive_mode was enabled at server start and
-archive_library is set to an empty string.
+archive_library is set to an empty string.  If both
+archive_command and archive_library
+are set, archiving will fail.
 If archive_command is an empty string (the default) while
 archive_mode is enabled (and archive_library
 is set to an empty string), WAL archiving is temporarily
@@ -3624,7 +3626,9 @@ include_dir 'conf.d'

 The library to use for archiving completed WAL file segments.  If set to
 an empty string (the default), archiving via shell is enabled, and
- is used.  Otherwise, the specified
+ is used.  If both
+archive_command and archive_library
+are set, archiving will fail.  Otherwise, the specified
 shared library is used for archiving.  For more information, see
  and
 .
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 3868cd7bd3..56dcc0dce5 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -838,6 +838,12 @@ LoadArchiveLibrary(void)
 
 	memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
 
+	if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("both archive_command and archive_library specified"),
+ errdetail("Only one of archive_command, archive_library may be set.")));
+
 	/*
 	 * If shell archiving is enabled, use our special initialization function.
 	 * Otherwise, load the library and call its _PG_archive_module_init().

Re: Add meson.build to version_stamp.pl

[Andres Freund]

Hi,

On 2022-10-05 11:36:36 +0100, Dagfinn Ilmari Mannsåker wrote:
> Peter Eisentraut  writes:
> 
> > Thinking ahead a bit, we need to add meson.build to version_stamp.pl.

Good idea.  I was wondering for a moment whether we should put the version
into some file instead, and reference that from meson.build - but the
convenient way to do that is only in 0.56, which we currently don't want to
require...  We could work around it, but it doesn't seem necessary for now.


> > Maybe someone can think of a better sed expression, but this one seems
> > good enough to me for now.
> […]
> > +sed_file("meson.build",
> > +   qq{-e "1,20s/ version: '[0-9a-z.]*',/ version: '$fullversion',/"}
> > +);
> 
> I think it would be better to not rely on the line numbers, but instead
> limit it to the project() stanza, something like this:

> sed_file("meson.build",
>   qq{-e "/^project(/,/^)/ s/ version: '[0-9a-z.]*',/ version: 
> '$fullversion',/"}
> );

Yea, that looks nicer.

Greetings,

Andres Freund

Re: Suppressing useless wakeups in walreceiver

[Nathan Bossart]

Thanks for taking a look.

On Wed, Oct 05, 2022 at 09:57:00AM +0200, Alvaro Herrera wrote:
> On 2022-Oct-04, Nathan Bossart wrote:
>> * The creation of the struct for non-shared WAL receiver state is moved to
>> a prerequisite 0001 patch.  This should help ease review of 0002 a bit.
> 
> I think that would be even better if you moved the API adjustments of
> some functions for the new struct to 0001 as well
> (XLogWalRcvSendHSFeedback etc).

I moved as much as I could to 0001 in v4.

>> * I updated the nap time calculation to round up to the next millisecond,
>> as discussed upthread.
> 
> I didn't look at this part very carefully, but IIRC walreceiver's
> responsivity has a direct impact on latency for sync replicas.  Maybe
> it'd be sensible to the definition that the sleep time is rounded down
> rather than up?  (At least, for the cases where we have something to do
> and not merely continue sleeping.) 

The concern is that if we wake up early, we'll end up spinning until the
wake-up time is reached.  Given the current behavior is to sleep for 100ms
at a time, and the tasks in question are governed by wal_receiver_timeout
and wal_receiver_status_interval (which are typically set to several
seconds) it seems reasonably safe to sleep up to an extra ~1ms here and
there without sacrificing responsiveness.  In fact, I imagine this patch
results in a net improvement in responsiveness for these tasks.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 0242daf5622cf649ad88975ce991c851388f8c99 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Wed, 5 Oct 2022 10:31:35 -0700
Subject: [PATCH v4 1/2] Move WAL receivers' non-shared state to a new struct.

This is preparatory work for a follow-up change that will revamp
the wakeup mechanism for periodic tasks that WAL receivers must
perform.
---
 src/backend/replication/walreceiver.c | 90 ++-
 1 file changed, 48 insertions(+), 42 deletions(-)

diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 6cbb67c92a..89985c54cf 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -116,6 +116,14 @@ static struct
 	XLogRecPtr	Flush;			/* last byte + 1 flushed in the standby */
 }			LogstreamResult;
 
+/*
+ * A struct to keep track of non-shared state.
+ */
+typedef struct WalRcvInfo
+{
+	TimeLineID	startpointTLI;
+} WalRcvInfo;
+
 static StringInfoData reply_message;
 static StringInfoData incoming_message;
 
@@ -123,12 +131,12 @@ static StringInfoData incoming_message;
 static void WalRcvFetchTimeLineHistoryFiles(TimeLineID first, TimeLineID last);
 static void WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI);
 static void WalRcvDie(int code, Datum arg);
-static void XLogWalRcvProcessMsg(unsigned char type, char *buf, Size len,
- TimeLineID tli);
-static void XLogWalRcvWrite(char *buf, Size nbytes, XLogRecPtr recptr,
-			TimeLineID tli);
-static void XLogWalRcvFlush(bool dying, TimeLineID tli);
-static void XLogWalRcvClose(XLogRecPtr recptr, TimeLineID tli);
+static void XLogWalRcvProcessMsg(WalRcvInfo *state, unsigned char type,
+ char *buf, Size len);
+static void XLogWalRcvWrite(WalRcvInfo *state, char *buf, Size nbytes,
+			XLogRecPtr recptr);
+static void XLogWalRcvFlush(WalRcvInfo *state, bool dying);
+static void XLogWalRcvClose(WalRcvInfo *state, XLogRecPtr recptr);
 static void XLogWalRcvSendReply(bool force, bool requestReply);
 static void XLogWalRcvSendHSFeedback(bool immed);
 static void ProcessWalSndrMessage(XLogRecPtr walEnd, TimestampTz sendTime);
@@ -175,7 +183,6 @@ WalReceiverMain(void)
 	char		slotname[NAMEDATALEN];
 	bool		is_temp_slot;
 	XLogRecPtr	startpoint;
-	TimeLineID	startpointTLI;
 	TimeLineID	primaryTLI;
 	bool		first_stream;
 	WalRcvData *walrcv = WalRcv;
@@ -185,6 +192,7 @@ WalReceiverMain(void)
 	char	   *err;
 	char	   *sender_host = NULL;
 	int			sender_port = 0;
+	WalRcvInfo	state = {0};
 
 	/*
 	 * WalRcv should be set up already (if we are a backend, we inherit this
@@ -238,7 +246,7 @@ WalReceiverMain(void)
 	strlcpy(slotname, (char *) walrcv->slotname, NAMEDATALEN);
 	is_temp_slot = walrcv->is_temp_slot;
 	startpoint = walrcv->receiveStart;
-	startpointTLI = walrcv->receiveStartTLI;
+	state.startpointTLI = walrcv->receiveStartTLI;
 
 	/*
 	 * At most one of is_temp_slot and slotname can be set; otherwise,
@@ -258,7 +266,7 @@ WalReceiverMain(void)
 	pg_atomic_write_u64(&WalRcv->writtenUpto, 0);
 
 	/* Arrange to clean up at walreceiver exit */
-	on_shmem_exit(WalRcvDie, PointerGetDatum(&startpointTLI));
+	on_shmem_exit(WalRcvDie, PointerGetDatum(&state));
 
 	/* Properly accept or ignore signals the postmaster might send us */
 	pqsignal(SIGHUP, SignalHandlerForConfigReload); /* set flag to read config
@@ -345,11 +353,11 @@ WalReceiverMain(void)
 		 * Confirm that the current timeline of the primary is the same or
 		 * ahead of ours.
 		 */
-		if (primaryTL

Re: Move backup-related code to xlogbackup.c/.h

[Alvaro Herrera]

On 2022-Oct-05, Michael Paquier wrote:

> And FWIW, the SQL interfaces for pg_backup_start() and
> pg_backup_stop() could stay in xlogfuncs.c.  This has the advantage to
> centralize in the same file all the SQL-function-specific checks.

As I recall, that has the disadvantage that the API exposure is a bit
higher -- I mean, with the patch as originally posted, there was less
cross-inclusion of header files, but that is gone in the version Bharat
posted as reply to this.  I'm not sure if that's caused by *this*
comment, or even that it's terribly significant, but it seems worth
considering at least.

xlog.h is included by a lot of stuff, so it would be great if it
itself included the smallest set of other files possible.

... that said, looking at the chart in
https://doxygen.postgresql.org//xlog_8h.html looks like the only file
we'd avoid indirectly including is pgtime.h (in addition to xlogbackup.h
itself).


(It's strange that xlog.h seems to have become included into rel.h by
commit 848ef42bb8c7 that did not otherwise touch either rel.h nor xlog.h.)

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

Re: archive modules

[Peter Eisentraut]

On 23.09.22 18:14, Nathan Bossart wrote:

On Fri, Sep 23, 2022 at 05:58:42AM -0400, Peter Eisentraut wrote:

On 15.09.22 00:27, Nathan Bossart wrote:

Both archive_command and archive_library are PGC_SIGHUP, so IIUC that
wouldn't be sufficient.  I attached a quick sketch that seems to provide
the desired behavior.  It's nowhere near committable yet, but it
demonstrates what I'm thinking.


What is the effect of issuing a warning like in this patch?  Would it just
not archive anything until the configuration is fixed?  I'm not sure what
behavior you are going for; it's a bit hard to imagine from just reading the
patch.


Yes, it will halt archiving and emit a WARNING, just like what happens on
released versions when you leave archive_command empty.


Leaving archive_command empty is an intentional configuration choice.

What we are talking about here is, arguably, a misconfiguration, so it 
should result in an error.

Re: installcheck-world concurrency issues

[Andres Freund]

Hi,

On 2022-10-05 08:16:37 +0200, Peter Eisentraut wrote:
> On 04.10.22 01:41, Andres Freund wrote:
> > BTW, shouldn't src/test/modules/unsafe_tests use the PG_TEST_EXTRA mechanism
> > somehow?  Seems not great to run it as part of installcheck-world, if we 
> > don't
> > want to run it as part of installcheck.
> 
> I think there are different levels and kinds of unsafeness. The ssl and
> kerberos tests start open server processes on your machine.  The
> modules/unsafe_tests just make a mess of your postgres instance.  The latter
> isn't a problem when run against a temp instance.

I agree - but I suspect our definition of danger is reversed. For me breaking
an existing cluster is a lot more likely to incur "real world" danger than
starting a throway instance listening to tcp on localhost...

Greetings,

Andres Freund

Re: [RFC] building postgres with meson - v13

[Andres Freund]

Hi,

On 2022-10-05 10:16:06 +0200, Peter Eisentraut wrote:
> On 04.10.22 05:25, Andres Freund wrote:
> > I've attached a revised version of the xml-tools dependency wrapper (0001):
> > Cleanups, minor error handling improvements, and bit of comment polishing. 
> > I'd
> > welcome review. But as it fixes a build-dependency bug / FIXME, I'm planning
> > to push it relatively soon otherwise.
> > 
> > 0002 fixes libpq's .pc file (static dependencies didn't show up anymore) and
> > AIX compilation. AIX doesn't yet support link_whole (support was merged into
> > meson yesterday though). On the way it also improves comments and a bit of
> > generic infrastructure.  The price for now is that the static libpq is built
> > separately from the shared one, not reusing any objects. I felt that the
> > complexity of reusing the objects isn't worth it for now.
> > 
> > Peter, it'd be great if you could have a look at 0002.
> > 
> > 0003 mirrors the setup of libpq to the various ecpg libraries. This is a
> > prerequisite to adding resource files.
> > 
> > 0004 adds the resource files
> 
> These patches look ok to me.

Thanks for checking.

With that I'm closing the original meson CF entry. Wohoo!

I'll post two new threads, one about pgxs compatibility, one about precompiled
headers in a bit.

Greetings,

Andres Freund

Re: START_REPLICATION SLOT causing a crash in an assert build

[Jonathan S. Katz]

On 9/27/22 1:52 AM, Kyotaro Horiguchi wrote:

Thanks!

At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund  wrote in

I wonder if the correct fix here wouldn't be to move the slotname out of
PgStat_StatReplSlotEntry?


Ugh. Right. I thought its outer struct as purely the part for the
common header. But we can freely place anything after the header
part. I moved it to the outer struct. I didn't clear that part in
pgstat_create_relation() because it is filled in immediately.

The attached is that.


This is still listed as an open item[1] for v15. Does this fix proposed 
address the issue?


Thanks,

Jonathan

[1] https://wiki.postgresql.org/wiki/PostgreSQL_15_Open_Items



OpenPGP_signature
Description: OpenPGP digital signature

Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)

[Alvaro Herrera]

Backpatching this to 12 shows yet another problem -- the topmost
relation acquires additional FK constraints, not yet sure why.  I think
we must have fixed something in 13 that wasn't backpatched, but I can't
remember what it is and whether it was intentionally not backpatched.

Looking ...

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I can see support will not be a problem.  10 out of 10."(Simon Wittber)
  (http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)

Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records

[Tom Lane]

Kyotaro Horiguchi  writes:
> At Tue, 4 Oct 2022 17:15:31 -0700, Nathan Bossart  
> wrote in 
>> On Tue, Oct 04, 2022 at 07:53:11PM -0400, Tom Lane wrote:
>>> After further thought, maybe it'd be better to do it as attached,
>>> with one long-lived hash table for all the locks.

> First one is straight forward outcome from the current implement but I
> like the new one.  I agree that it is natural and that the expected
> overhead per (typical) transaction is lower than both the first one
> and doing the same operation on a list. I don't think that space
> inefficiency in that extent doesn't matter since it is the startup
> process.

To get some hard numbers about this, I made a quick hack to collect
getrusage() numbers for the startup process (patch attached for
documentation purposes).  I then ran the recovery/t/027_stream_regress.pl
test a few times and collected the stats (also attached).  This seems
like a reasonably decent baseline test, since the core regression tests
certainly take lots of AccessExclusiveLocks what with all the DDL
involved, though they shouldn't ever take large numbers at once.  Also
they don't run long enough for any lock list bloat to occur, so these
numbers don't reflect a case where the patches would provide benefit.

If you look hard, there's maybe about a 1% user-CPU penalty for patch 2,
although that's well below the run-to-run variation so it's hard to be
sure that it's real.  The same comments apply to the max resident size
stats.  So I'm comforted that there's not a significant penalty here.

I'll go ahead with patch 2 if there's not objection.

One other point to discuss: should we consider back-patching?  I've
got mixed feelings about that myself.  I don't think that cases where
this helps significantly are at all mainstream, so I'm kind of leaning
to "patch HEAD only".

regards, tom lane

diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index f99186eab7..8fcbf898f9 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -32,6 +32,7 @@
 #include "storage/pmsignal.h"
 #include "storage/procsignal.h"
 #include "storage/standby.h"
+#include "tcop/tcopprot.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/timeout.h"
@@ -219,6 +220,8 @@ StartupProcExit(int code, Datum arg)
 	/* Shutdown the recovery environment */
 	if (standbyState != STANDBY_DISABLED)
 		ShutdownRecoveryTransactionEnvironment();
+
+	ShowUsage("STARTUP PROCESS STATISTICS");
 }
 
 
@@ -229,6 +232,8 @@ StartupProcExit(int code, Datum arg)
 void
 StartupProcessMain(void)
 {
+	ResetUsage();
+
 	/* Arrange to clean up at startup process exit */
 	on_shmem_exit(StartupProcExit, 0);
 
Startup process stats from 027_stream_regress_standby_1.log

HEAD, --disable-cassert:

2022-10-05 10:57:15.561 EDT [2390709] LOG:  STARTUP PROCESS STATISTICS
2022-10-05 10:57:15.561 EDT [2390709] DETAIL:  ! system usage stats:
!   0.935814 s user, 1.019952 s system, 7.226729 s elapsed
!   [0.936104 s user, 1.019952 s system total]
!   9088 kB max resident size
!   0/430608 [0/430608] filesystem blocks in/out
!   0/916 [0/1026] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   9925/180 [9925/180] voluntary/involuntary context switches

2022-10-05 11:02:58.542 EDT [2406908] LOG:  STARTUP PROCESS STATISTICS
2022-10-05 11:02:58.542 EDT [2406908] DETAIL:  ! system usage stats:
!   0.957075 s user, 1.029002 s system, 7.206675 s elapsed
!   [0.957075 s user, 1.029260 s system total]
!   9124 kB max resident size
!   0/430512 [0/430512] filesystem blocks in/out
!   0/919 [0/1028] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   9575/174 [9575/175] voluntary/involuntary context switches

2022-10-05 11:03:35.436 EDT [2409026] LOG:  STARTUP PROCESS STATISTICS
2022-10-05 11:03:35.436 EDT [2409026] DETAIL:  ! system usage stats:
!   0.936716 s user, 1.117313 s system, 7.208555 s elapsed
!   [0.936716 s user, 1.117606 s system total]
!   9220 kB max resident size
!   0/429712 [0/429712] filesystem blocks in/out
!   0/917 [0/1028] page faults/reclaims, 0 [0] swaps
!   0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
!   9509/178 [9509/178] voluntary/involuntary context switches

with Patch 1:

2022-10-05 10:58:50.088 EDT [2393477] LOG:  STARTUP PROCESS STATISTICS
2022-10-05 10:58:50.088 EDT [2393477] DETAIL:  ! system usage stats:
!   0.938248 s user, 1.103261 s system, 7.269850 s elapsed
!   [0.938498 s user, 1.103261 s system total]
!   9288 kB max resident size
!   0/430912 [0/430912] filesystem blocks in/out
!   0/936 [0/1046] page faults/reclaims, 

RE: Data is copied twice when specifying both child and parent table in publication

[osumi.takami...@fujitsu.com]

On Wednesday, September 28, 2022 5:36 PM Wang, Wei/王 威  
wrote:
> Also rebased the patch because the change in the HEAD (20b6847).
> 
> Attach the new patches.
Hi, thank you for the updated patches!


Here are my minor review comments for HEAD v12.

(1) typo & suggestion to reword one comment


+* Publications support partitioned tables. If
+* publish_via_partition_root is false, all changes are 
replicated
+* using leaf partition identity and schema, so we only 
need
+* those. Otherwise, If publish_via_partition_root is 
true, get
+* the partitioned table itself.


The last sentence has "If" in the middle of the sentence.
We can use the lower letter for it. Or, I think "Otherwise" by itself means
"If publish_via_partition_root is true". So, I'll suggest a below change.


FROM:
Otherwise, If publish_via_partition_root is true, get the partitioned table 
itself.
TO:
Otherwise, get the partitioned table itself.


(2) Do we need to get "attnames" column from the publisher in the 
fetch_table_list() ?

When I was looking at v16 path, I didn't see any codes that utilize
the "attnames" column information returned from the publisher.
If we don't need it, could we remove it ?
I can miss something greatly, but this might be affected by HEAD codes ?



Best Regards,
Takamichi Osumi

Re: shadow variables - pg15 edition

[Tom Lane]

David Rowley  writes:
> I've attached a draft patch for a method I was considering to fix the
> warnings we're getting from the nested PG_TRY() statement in
> utility.c.

+1

> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2".

Yeah, "now2" seems as reasonable as anything.

regards, tom lane

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

[Zhihong Yu]

On Wed, Oct 5, 2022 at 4:38 AM Andrey Lepikhov 
wrote:

> On 5/10/2022 02:45, Zhihong Yu wrote:
> > Hi,
> > For contain_placeholders():
> >
> > +   if (IsA(node, Query))
> > +   return query_tree_walker((Query *) node, contain_placeholders,
> > context, 0);
> > +   else if (IsA(node, PlaceHolderVar))
> Fixed
> >
> > The `else` is not needed.
> >
> > For correlated_t struct, it would be better if the fields have comments.
> Ok, I've added some comments.
> >
> > +* (for grouping, as an example). So, revert its
> > status to
> > +* a full valued entry.
> >
> > full valued -> fully valued
> Fixed
>
> --
> regards,
> Andrey Lepikhov
> Postgres Professional
>
Hi,

+   List   *pulling_quals; /* List of expressions contained pulled
expressions */

contained -> containing

+   /* Does the var already exists in the target list? */

 exists -> exist

+   {"optimize_correlated_subqueries", PGC_USERSET, QUERY_TUNING_METHOD,

Is it possible that in the future there would be other optimization for
correlated subqueries ?
If so, either rename the guc or, make the guc a string which represents an
enum.

Cheers

Re: Can we avoid chdir'ing in resolve_symlinks() ?

[Tom Lane]

Peter Eisentraut  writes:
> To clarify, this instance is not at all the fault of any code in 
> PostgreSQL.  But it's another instance where resolving symlinks just 
> because we can causing problems.

[ shrug... ]  *Not* resolving symlinks when we can causes its
own set of problems, which maybe we don't see very clearly
because we have been doing it like that for a couple of decades.
I remain pretty hesitant to change this behavior.

What did you think of the compromise proposal to change only
the paths that pg_config outputs?  I've not tried to code that,
but I think it should be feasible.

regards, tom lane

Re: shadow variables - pg15 edition

[Alvaro Herrera]

On 2022-Oct-05, David Rowley wrote:

> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509.  I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2". It's just I didn't like that as it sort
> of implies there are multiple definitions of "now" and I struggle with
> that...  maybe I'm just thinking too much in terms of Newtonian
> Relativity...

:-D

A simpler idea might be to just remove the inner declaration, and have
that block set the outer var.  There's no damage, since the block is
going to end and not access the previous value anymore.

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index aa1a3541fe..91a067859b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -7506,7 +7506,7 @@ threadRun(void *arg)
/* progress report is made by thread 0 for all threads */
if (progress && thread->tid == 0)
{
-   pg_time_usec_t now = pg_time_now();
+   now = pg_time_now();/* not lazy; clobbers outer 
value */
 
if (now >= next_report)
{


The "now now" reference reminded me of "ahorita"
https://doorwaytomexico.com/paulina/ahorita-meaning-examples/
which is source of misunderstandings across borders in South America ...

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."(Freeman Dyson)

Re: [PoC] Let libpq reject unexpected authentication requests

[Peter Eisentraut]

On 23.09.22 02:02, Jacob Champion wrote:

On Thu, Sep 22, 2022 at 4:52 AM Peter Eisentraut
 wrote:

On 22.09.22 01:37, Jacob Champion wrote:

I think this is potentially
dangerous, but it mirrors the current behavior of libpq and I'm not
sure that we should change it as part of this patch.


It might be worth reviewing that behavior for other reasons, but I think
semantics of your patch are correct.


Sounds good. v9 removes the TODO and adds a better explanation.


I'm generally okay with these patches now.


I'm not able to test SSPI easily at the moment; if anyone is able to
try it out, that'd be really helpful. There's also the question of
SASL forwards compatibility -- if someone adds a new SASL mechanism,
the code will treat it like scram-sha-256 until it's changed, and
there will be no test to catch it. Should we leave that to the future
mechanism implementer to fix, or add a mechanism check now so the
client is safe even if they forget?


I think it would be good to put some provisions in place here, even if 
they are elementary.  Otherwise, there will be a significant burden on 
the person who implements the next SASL method (i.e., you ;-) ) to 
figure that out then.


I think you could just stick a string list of allowed SASL methods into 
PGconn.


By the way, I'm not sure all the bit fiddling is really worth it.  An 
array of integers (or unsigned char or whatever) would work just as 
well.  Especially if you are going to have a string list for SASL 
anyway.  You're not really saving any bits or bytes either way in the 
normal case.


Minor comments:

Pasting together error messages like with auth_description() isn't going 
to work.  You either need to expand the whole message in 
check_expected_areq(), or perhaps rephrase the message like


libpq_gettext("auth method \"%s\" required, but server requested \"%s\"\n"),
 conn->require_auth,
 auth_description(areq)

and make auth_description() just return a single word not subject to 
translation.


spurious whitespace change in fe-secure-openssl.c

whitespace error in patch:

.git/rebase-apply/patch:109: tab in indent.
via TLS, nor GSS authentication via its 
encrypted transport.)


In the 0002 patch, the configure test needs to be added to meson.build.

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

[Drouvot, Bertrand]

Hi,

On 10/5/22 9:24 AM, Michael Paquier wrote:

Something that stood out on a first review is the refactoring of
001_password.pl that can be done independently of the main patch:


Good idea, thanks for the proposal.


- test_role() -> test_conn() to be able to pass down a database name.
- reset_pg_hba() to control the host, db and user parts.  The host
part does not really apply after moving the hosts checks to a more
secure location, so I guess that this had better be extended just for
the user and database, keeping host=local all the time.
I am planning to apply 0001 attached independently, 


0001 looks good to me.


reducing the
footprint of 0002, which is your previous patch left untouched
(mostly!).


Thanks! I'll look at it and the comments you just made up-thread.

Regards,

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

Re: installing static libraries (was building postgres with meson)

[Peter Eisentraut]

On 04.10.22 00:42, Andres Freund wrote:

I realize that there are people for whom other considerations outweigh
that, but I don't think that we should install static libraries by
default.  Long ago it was pretty common for configure scripts to
offer --enable-shared and --enable-static options ... should we
resurrect that?


It'd be easy enough. I don't really have an opinion on whether it's worth
having the options. I think most packaging systems have ways of not including
files even if $software installs them.


Right.  I think there is enough work to stabilize and synchronize the 
new build system.  I don't really see a need to prioritize this.



A few questions, in case we want to do this:

1) should this affect libraries we build only as static libraries, like
pgport, pgcommon, pgfeutils?

I assume there's some extensions that build binaries with pgxs, which then
presumably need pgport, pgcommon.


I'm not familiar with cases like this and what their expectations would be.


2) Would we want the option add it to autoconf and meson, or just meson?


if at all, then both


3) For meson, I'd be inclined to leave the static libraries in as build
targets, but just not build and install them by default.


not sure why


4) Why are we installing the static libraries into libdir? Given that they're
not versioned at all, it somehow seems pkglibdir would be more appropriate?


That's the standard file system layout.  I don't think we need to 
editorialize that.

Re: Add non-blocking version of PQcancel

[Jelte Fennema]

Thanks for all the feedback. I attached a new patch that I think
addresses all of it. Below some additional info.

> On the whole it feels like a mistake to have two separate kinds of
> PGconn with fundamentally different behaviors and yet no distinction
> in the API.  I think I'd recommend having a separate struct type
> (which might internally contain little more than a pointer to a
> cloned PGconn), and provide only a limited set of operations on it.

In my first version of this patch, this is exactly what I did. But then
I got this feedback from Jacob, so I changed it to reusing PGconn:

> >  /* issue a cancel request */
> >  extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
> > +extern PGcancelConn * PQcancelConnectStart(PGconn *conn);
> > +extern PGcancelConn * PQcancelConnect(PGconn *conn);
> > +extern PostgresPollingStatusType PQcancelConnectPoll(PGcancelConn * 
> > cancelConn);
> > +extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
> > +extern int PQcancelSocket(const PGcancelConn * cancelConn);
> > +extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
> > +extern void PQcancelFinish(PGcancelConn * cancelConn);
>
> That's a lot of new entry points, most of which don't do anything
> except call their twin after a pointer cast. How painful would it be to
> just use the existing APIs as-is, and error out when calling
> unsupported functions if conn->cancelRequest is true?

I changed it back to use PGcancelConn as per your suggestion and I 
agree that the API got better because of it.

> +   CONNECTION_CANCEL_FINISHED,
>    /* Non-blocking mode only below here */
> 
> is an absolute non-starter: it breaks ABI for every libpq client,
> even ones that aren't using this facility. 

I removed this now. The main reason was so it was clear that no
queries could be sent over the connection, like is normally the case
when CONNECTION_OK happens. I don't think this is as useful anymore
now that this patch has a dedicated PGcancelStatus function.
NOTE: The CONNECTION_STARTING ConnStatusType is still necessary.
But to keep ABI compatibility I moved it to the end of the enum.

> Alternatively, we could teach libpq_pipeline
> to do getenv("PG_TEST_TIMEOUT_DEFAULT") with a fallback to 180,
> but that feels like it might be overly familiar with the innards
> of Utils.pm.

I went with this approach, because this environment variable was
already used in 2 other places than Utils.pm: 
- contrib/test_decoding/sql/twophase.sql
- src/test/isolation/isolationtester.c

So, one more place seemed quite harmless.

P.S. I noticed a logical conflict between this patch and my libpq load 
balancing patch. Because this patch depends on the connhost array 
is constructed the exact same on the second invocation of connectOptions2.
But the libpq loadbalancing patch breaks this assumption. I'm making
a mental (and public) note that whichever of these patches gets merged last
should address this issue.   

0001-Add-non-blocking-version-of-PQcancel.patch
Description: 0001-Add-non-blocking-version-of-PQcancel.patch

Re: [PATCH] Log details for client certificate failures

[Ranier Vilela]

Em qua., 5 de out. de 2022 às 09:19, Ranier Vilela 
escreveu:

> Hi,
> I think that 9d58bf
> ,
> left a tiny oversight.
>
> guc_strdup uses strdup, so must be cleaned by free not pfree.
> IMO, can be used once free call too.
>
Sorry, my fault.
Please ignore this.

regards,
Ranier Vilela

Re: Removing unneeded self joins

[Andrey Lepikhov]

New version, rebased onto current master.
Nothing special, just rebase.

--
regards,
Andrey Lepikhov
Postgres Professional
From 03aab7a2431032166c9ea5f52fbcccaf7168abec Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" 
Date: Wed, 5 Oct 2022 16:58:34 +0500
Subject: [PATCH] Remove self-joins.

A Self Join Removal (SJR) feature removes inner join of plain table to itself
in a query plan if can be proved that the join can be replaced with a scan.
The proof based on innerrel_is_unique machinery.

We can remove a self-join when for each outer row:
1. At most one inner row matches the join clauses.
2. If the join target list contains any inner vars, an inner row
must be (physically) the same row as the outer one.

In this patch we use Rowley's [1] approach to identify a self-join:
1. Collect all mergejoinable join quals which look like a.x = b.x
2. Check innerrel_is_unique() for the qual list from (1). If it
returns true, then outer row matches only the same row from the inner
relation.
3. If uniqueness of outer relation deduces from baserestrictinfo clause like
'x=const' on unique field(s), check that inner has the same clause with the
same constant(s).
4. Compare RowMarks of inner and outer relations. They must have the same
strength.

Some regression tests changed due to self-join removal logic.

[1] 
https://www.postgresql.org/message-id/raw/CAApHDvpggnFMC4yP-jUO7PKN%3DfXeErW5bOxisvJ0HvkHQEY%3DWw%40mail.gmail.com
---
 doc/src/sgml/config.sgml  |   16 +
 src/backend/optimizer/path/indxpath.c |   38 +
 src/backend/optimizer/plan/analyzejoins.c | 1046 -
 src/backend/optimizer/plan/planmain.c |5 +
 src/backend/utils/misc/guc_tables.c   |   22 +
 src/include/optimizer/paths.h |3 +
 src/include/optimizer/planmain.h  |7 +
 src/test/regress/expected/equivclass.out  |   32 +
 src/test/regress/expected/join.out|  774 +++
 src/test/regress/expected/sysviews.out|3 +-
 src/test/regress/sql/equivclass.sql   |   16 +
 src/test/regress/sql/join.sql |  340 +++
 src/tools/pgindent/typedefs.list  |2 +
 13 files changed, 2278 insertions(+), 26 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d750290f13..5ce2d4d2fa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5290,6 +5290,22 @@ ANY num_sync ( 
+  enable_self_join_removal (boolean)
+  
+   enable_self_join_removal configuration 
parameter
+  
+  
+  
+   
+Enables or disables the query planner's optimization which analyses
+query tree and replaces self joins with semantically equivalent single
+scans. Take into consideration only plain tables.
+The default is on.
+   
+  
+ 
+
  
   enable_seqscan (boolean)
   
diff --git a/src/backend/optimizer/path/indxpath.c 
b/src/backend/optimizer/path/indxpath.c
index c31fcc917d..51f672a65c 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -3513,6 +3513,21 @@ bool
 relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
  List *restrictlist,
  List *exprlist, List 
*oprlist)
+{
+   return relation_has_unique_index_ext(root, rel, restrictlist,
+   
 exprlist, oprlist, NULL);
+}
+
+/*
+ * relation_has_unique_index_ext
+ * if extra_clauses isn't NULL, return baserestrictinfo clauses which were
+ * used to derive uniqueness.
+ */
+bool
+relation_has_unique_index_ext(PlannerInfo *root, RelOptInfo *rel,
+ List *restrictlist,
+ List *exprlist, List 
*oprlist,
+ List **extra_clauses)
 {
ListCell   *ic;
 
@@ -3568,6 +3583,7 @@ relation_has_unique_index_for(PlannerInfo *root, 
RelOptInfo *rel,
{
IndexOptInfo *ind = (IndexOptInfo *) lfirst(ic);
int c;
+   List   *exprs = NIL;
 
/*
 * If the index is not unique, or not immediately enforced, or 
if it's
@@ -3616,6 +3632,24 @@ relation_has_unique_index_for(PlannerInfo *root, 
RelOptInfo *rel,
if (match_index_to_operand(rexpr, c, ind))
{
matched = true; /* column is unique */
+
+   if 
(bms_membership(rinfo->clause_relids) == BMS_SINGLETON)
+   {
+   MemoryContext oldMemCtx =
+   
MemoryContextSwitchTo(root->plann

Re: [PATCH] Log details for client certificate failures

[Ranier Vilela]

Hi,
I think that 9d58bf
,
left a tiny oversight.

guc_strdup uses strdup, so must be cleaned by free not pfree.
IMO, can be used once free call too.

regards,
Ranier Vilela


simplifies_and_use_free_variable.patch
Description: Binary data

Re: Miscellaneous tab completion issue fixes

[vignesh C]

On Wed, 5 Oct 2022 at 08:17, Michael Paquier  wrote:
>
> On Tue, Oct 04, 2022 at 10:35:25AM +0100, Dagfinn Ilmari Mannsåker wrote:
> > LGTM, +1 to commit.
>
> Fine by me, so done.

Thanks for pushing this.

Regards,
Vignesh

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

[Andrey Lepikhov]

On 5/10/2022 02:45, Zhihong Yu wrote:

Hi,
For contain_placeholders():

+   if (IsA(node, Query))
+       return query_tree_walker((Query *) node, contain_placeholders, 
context, 0);

+   else if (IsA(node, PlaceHolderVar))

Fixed


The `else` is not needed.

For correlated_t struct, it would be better if the fields have comments.

Ok, I've added some comments.


+                    * (for grouping, as an example). So, revert its 
status to

+                    * a full valued entry.

full valued -> fully valued

Fixed

--
regards,
Andrey Lepikhov
Postgres Professional
From da570914e53bc34e6bf3291649725a041a8b929c Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" 
Date: Wed, 28 Sep 2022 13:42:12 +0300
Subject: [PATCH] Transform correlated subquery of type N-J [1] into ordinary
 join query. With many restrictions, check each subquery and pull up
 expressions, references upper query block. Works for operators '=' and 'IN'.
 Machinery of converting ANY subquery to JOIN stays the same with minor
 changes in walker function.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Pass a lower outer join through the pullup_sublink recursive procedure to find
out some situations when qual references outer side of an outer join.

[1] Kim, Won. “On optimizing an SQL-like nested query.” ACM Trans. Database 
Syst. 7 (1982): 443-469.
---
 src/backend/optimizer/plan/subselect.c| 328 +++-
 src/backend/optimizer/prep/prepjointree.c |  71 ++--
 src/backend/optimizer/util/tlist.c|   2 +-
 src/backend/optimizer/util/var.c  |   8 +
 src/backend/utils/misc/guc_tables.c   |  10 +
 src/include/optimizer/optimizer.h |   1 +
 src/include/optimizer/subselect.h |   3 +-
 src/include/optimizer/tlist.h |   1 +
 src/test/regress/expected/prepare.out |  18 +
 src/test/regress/expected/subselect.out   | 438 ++
 src/test/regress/sql/prepare.sql  |   7 +
 src/test/regress/sql/subselect.sql| 162 
 src/tools/pgindent/typedefs.list  |   1 +
 13 files changed, 1013 insertions(+), 37 deletions(-)

diff --git a/src/backend/optimizer/plan/subselect.c 
b/src/backend/optimizer/plan/subselect.c
index 92e3338584..be19d85524 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -32,6 +32,7 @@
 #include "optimizer/planner.h"
 #include "optimizer/prep.h"
 #include "optimizer/subselect.h"
+#include "optimizer/tlist.h"
 #include "parser/parse_relation.h"
 #include "rewrite/rewriteManip.h"
 #include "utils/builtins.h"
@@ -65,6 +66,8 @@ typedef struct inline_cte_walker_context
 } inline_cte_walker_context;
 
 
+bool optimize_correlated_subqueries = true;
+
 static Node *build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
   List *plan_params,
   SubLinkType subLinkType, int 
subLinkId,
@@ -1229,6 +1232,296 @@ inline_cte_walker(Node *node, inline_cte_walker_context 
*context)
return expression_tree_walker(node, inline_cte_walker, context);
 }
 
+static bool
+contain_placeholders(Node *node, inline_cte_walker_context *context)
+{
+   if (node == NULL)
+   return false;
+
+   if (IsA(node, Query))
+   return query_tree_walker((Query *) node, contain_placeholders, 
context, 0);
+   if (IsA(node, PlaceHolderVar))
+   return true;
+
+   return expression_tree_walker(node, contain_placeholders, context);
+}
+
+/*
+ * To be pullable all clauses of flattening correlated subquery should be
+ * anded and mergejoinable (XXX: really necessary?)
+ */
+static bool
+quals_is_pullable(Node *quals)
+{
+   if (!contain_vars_of_level(quals, 1))
+   return true;
+
+   if (quals && IsA(quals, OpExpr))
+   {
+   OpExpr *expr = (OpExpr *) quals;
+   Node   *leftarg;
+
+   /* Contains only one expression */
+   leftarg = linitial(expr->args);
+   if (!op_mergejoinable(expr->opno, exprType(leftarg))) /* Is it 
really necessary ? */
+   return false;
+
+   if (contain_placeholders(quals, NULL))
+   return false;
+
+   return true;
+   }
+   else if (is_andclause(quals))
+   {
+   ListCell   *l;
+
+   foreach(l, ((BoolExpr *) quals)->args)
+   {
+   Node *andarg = (Node *) lfirst(l);
+
+   if (!IsA(andarg, OpExpr))
+   return false;
+   if (!quals_is_pullable(andarg))
+   return false;
+   }
+
+   return true;
+   }
+
+   return false;
+}
+
+/*
+ * Mutator context used in pull-up process of expressions from WHERE quals.
+ */
+typedef struct
+{
+   Query  *subquery; /* Link

Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)

[Alvaro Herrera]

On 2022-Oct-03, Jehan-Guillaume de Rorthais wrote:

> Thank you! This is fixed and rebased on current master branch in patches
> attached.

Thanks.  As far as I can see this fixes the bugs that were reported.
I've been giving the patches a look and it caused me to notice two
additional bugs in the same area:

- FKs in partitions are sometimes marked NOT VALID.  This is because of
  missing initialization when faking up a Constraint node in
  CloneFkReferencing.  Easy to fix, have patch, running tests now.

- The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not
  correctly propagated.  This should be an easy fix also, haven't tried,
  need to add a test case.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

Re: Add meson.build to version_stamp.pl

[Dagfinn Ilmari Mannsåker]

Peter Eisentraut  writes:

> Thinking ahead a bit, we need to add meson.build to version_stamp.pl.
>
> Maybe someone can think of a better sed expression, but this one seems
> good enough to me for now.
[…]
> +sed_file("meson.build",
> + qq{-e "1,20s/ version: '[0-9a-z.]*',/ version: '$fullversion',/"}
> +);

I think it would be better to not rely on the line numbers, but instead
limit it to the project() stanza, something like this:

sed_file("meson.build",
qq{-e "/^project(/,/^)/ s/ version: '[0-9a-z.]*',/ version: 
'$fullversion',/"}
);

- ilmari

Re: shadow variables - pg15 edition

[David Rowley]

On Wed, 5 Oct 2022 at 21:05, David Rowley  wrote:
> 5 warnings remain.  4 of these are for PG_TRY() and co.

I've attached a draft patch for a method I was considering to fix the
warnings we're getting from the nested PG_TRY() statement in
utility.c.

The C preprocessor does not allow name overloading in macros, but of
course, it does allow variable argument marcos with ...  so I just
used that and added ##__VA_ARGS__ to each variable.  I think that
should work ok providing callers only supply 0 or 1 arguments to the
macro, and of course, make that parameter value the same for each set
of macros used in the PG_TRY() statement.

The good thing about the optional argument is that we don't need to
touch any existing users of PG_TRY().  The attached just modifies the
inner-most PG_TRY() in the only nested PG_TRY() we have in the tree in
utility.c.

The only warning remaining after applying the attached is the "now"
warning in pgbench.c:7509.  I'd considered changing this to "thenow"
which translates to "right now" in the part of Scotland that I'm from.
I also considered "nownow", which is used in South Africa [1].
Anyway, I'm not really being serious, but I didn't come up with
anything better than "now2". It's just I didn't like that as it sort
of implies there are multiple definitions of "now" and I struggle with
that...  maybe I'm just thinking too much in terms of Newtonian
Relativity...

David

[1] https://www.goodthingsguy.com/fun/now-now-just-now/
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index aa00815787..247d0816ad 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1678,16 +1678,16 @@ ProcessUtilitySlow(ParseState *pstate,
 * command itself is queued, which is enough.
 */
EventTriggerInhibitCommandCollection();
-   PG_TRY();
+   PG_TRY(2);
{
address = 
ExecRefreshMatView((RefreshMatViewStmt *) parsetree,

 queryString, params, qc);
}
-   PG_FINALLY();
+   PG_FINALLY(2);
{

EventTriggerUndoInhibitCommandCollection();
}
-   PG_END_TRY();
+   PG_END_TRY(2);
break;
 
case T_CreateTrigStmt:
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4dd9658a3c..c79d6cfba3 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -310,39 +310,48 @@ extern PGDLLIMPORT ErrorContextCallback 
*error_context_stack;
  * pedantry; we have seen bugs from compilers improperly optimizing code
  * away when such a variable was not marked.  Beware that gcc's -Wclobbered
  * warnings are just about entirely useless for catching such oversights.
+ *
+ * Each of these macros accepts an optional argument which can be specified
+ * to apply a suffix to the variables declared within the macros.  This suffix
+ * can be used to avoid the compiler emitting warnings about shadowed
+ * variables when compiling with -Wshadow in situations where nested PG_TRY()
+ * statements are required.  The optional suffix may contain any character
+ * that's allowed in a variable name.  The suffix, if specified must be the
+ * same within each set of PG_TRY() / PG_CATCH() / PG_FINALLY() / PG_END_TRY()
+ * statements.
  *--
  */
-#define PG_TRY()  \
+#define PG_TRY(...)  \
do { \
-   sigjmp_buf *_save_exception_stack = PG_exception_stack; \
-   ErrorContextCallback *_save_context_stack = 
error_context_stack; \
-   sigjmp_buf _local_sigjmp_buf; \
-   bool _do_rethrow = false; \
-   if (sigsetjmp(_local_sigjmp_buf, 0) == 0) \
+   sigjmp_buf *_save_exception_stack##__VA_ARGS__ = 
PG_exception_stack; \
+   ErrorContextCallback *_save_context_stack##__VA_ARGS__ = 
error_context_stack; \
+   sigjmp_buf _local_sigjmp_buf##__VA_ARGS__; \
+   bool _do_rethrow##__VA_ARGS__ = false; \
+   if (sigsetjmp(_local_sigjmp_buf##__VA_ARGS__, 0) == 0) \
{ \
-   PG_exception_stack = &_local_sigjmp_buf
+   PG_exception_stack = &_local_sigjmp_buf##__VA_ARGS__
 
-#define PG_CATCH() \
+#define PG_CATCH(...)  \
} \
else \
{ \
-   PG_exception_stack = _save_exception_stack; \
-   error_context_stack = _save_context_stack
+   PG_exception_stack = 
_save_exception_stack##__VA_ARGS__; \

Re: [Commitfest 2022-09] Date is Over.

[Julien Rouhaud]

Hi,

On Wed, Oct 05, 2022 at 02:50:58PM +0500, Ibrar Ahmed wrote:
> On Wed, 5 Oct 2022 at 1:43 PM, Alvaro Herrera 
> wrote:
> 
> > On 2022-Oct-03, Ibrar Ahmed wrote:
> >
> > > The date of the current commitfest is over, here is the current status of
> > > the  "September 2022 commitfest."
> > > There were 296 patches in the commitfest and 58 were get committed.
> >
> > Are you moving the open patches to the next commitfest, closing some as
> > RwF, etc?  I'm not clear what the status is, for the November commitfest.
> 
> 
> I am also not clear about that should I move that or wait till November.
> Anybody guide me

The CF should be marked as closed, and its entries fully processed within a few
days after its final day.

The general rule is that patches that have been waiting on authors for 2 weeks
or more without any answer from the author(s) should get returned with
feedback with some message to the author, and the rest is moved to the next
commitfest.  If you have the time to look at some patches and see if they need
something else, that's always better.

Re: Move backup-related code to xlogbackup.c/.h

[Bharath Rupireddy]

On Wed, Oct 5, 2022 at 1:20 PM Michael Paquier  wrote:
>
> On Tue, Oct 04, 2022 at 03:54:20PM -0700, Nathan Bossart wrote:
> > I would suggest moving this to a separate prerequisite patch that can be
> > reviewed independently from the patches that simply move code to a
> > different file.

I added the new functions in 0001 patch for ease of review.

> And FWIW, the SQL interfaces for pg_backup_start() and
> pg_backup_stop() could stay in xlogfuncs.c.  This has the advantage to
> centralize in the same file all the SQL-function-specific checks.

Agreed.

+extern void WALInsertLockAcquire(void);
+extern void WALInsertLockAcquireExclusive(void);
+extern void WALInsertLockRelease(void);
+extern void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);

Note that I had moved all WAL insert lock related functions to xlog.h
despite xlogbackup.c using 2 of them. This is done to keep all the
functions together.

Please review the attached v2 patch set.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 7a0359813790c5c3872b5308ff562371e6467266 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Wed, 5 Oct 2022 08:59:45 +
Subject: [PATCH v2] Add functions for xlogbackup.c to call back into xlog.c

---
 src/backend/access/transam/xlog.c | 119 --
 src/include/access/xlog.h |  17 +
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 27085b15a8..604220b474 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -697,10 +697,6 @@ static XLogRecPtr XLogBytePosToRecPtr(uint64 bytepos);
 static XLogRecPtr XLogBytePosToEndRecPtr(uint64 bytepos);
 static uint64 XLogRecPtrToBytePos(XLogRecPtr ptr);
 
-static void WALInsertLockAcquire(void);
-static void WALInsertLockAcquireExclusive(void);
-static void WALInsertLockRelease(void);
-static void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);
 
 /*
  * Insert an XLOG record represented by an already-constructed chain of data
@@ -1305,7 +1301,7 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
 /*
  * Acquire a WAL insertion lock, for inserting to WAL.
  */
-static void
+void
 WALInsertLockAcquire(void)
 {
 	bool		immed;
@@ -1350,7 +1346,7 @@ WALInsertLockAcquire(void)
  * Acquire all WAL insertion locks, to prevent other backends from inserting
  * to WAL.
  */
-static void
+void
 WALInsertLockAcquireExclusive(void)
 {
 	int			i;
@@ -1379,7 +1375,7 @@ WALInsertLockAcquireExclusive(void)
  * NB: Reset all variables to 0, so they cause LWLockWaitForVar to block the
  * next time the lock is acquired.
  */
-static void
+void
 WALInsertLockRelease(void)
 {
 	if (holdingAllLocks)
@@ -1405,7 +1401,7 @@ WALInsertLockRelease(void)
  * Update our insertingAt value, to let others know that we've finished
  * inserting up to that point.
  */
-static void
+void
 WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt)
 {
 	if (holdingAllLocks)
@@ -8983,3 +8979,110 @@ SetWalWriterSleeping(bool sleeping)
 	XLogCtl->WalWriterSleeping = sleeping;
 	SpinLockRelease(&XLogCtl->info_lck);
 }
+
+/*
+ * Get the ControlFile.
+ */
+ ControlFileData *
+ GetControlFile(void)
+ {
+	return ControlFile;
+ }
+
+/*
+ * Set the forcePageWrites flag.
+ */
+void
+SetforcePageWrites(bool need_lock, bool value)
+{
+	if (need_lock)
+		WALInsertLockAcquireExclusive();
+
+	XLogCtl->Insert.forcePageWrites = value;
+
+	if (need_lock)
+		WALInsertLockRelease();
+}
+
+/*
+ * Set the runningBackups value.
+ */
+void
+SetrunningBackups(bool need_lock, int value)
+{
+	if (need_lock)
+		WALInsertLockAcquireExclusive();
+
+	XLogCtl->Insert.runningBackups = value;
+
+	if (need_lock)
+		WALInsertLockRelease();
+}
+
+/*
+ * Get the runningBackups value.
+ */
+int
+GetrunningBackups(bool need_lock)
+{
+	int value;
+
+	if (need_lock)
+		WALInsertLockAcquireExclusive();
+
+	value = XLogCtl->Insert.runningBackups;
+
+	if (need_lock)
+		WALInsertLockRelease();
+
+	return value;
+}
+
+/*
+ * Get the lastFpwDisableRecPtr.
+ */
+XLogRecPtr
+GetlastFpwDisableRecPtr(void)
+{
+	XLogRecPtr recptr;
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	recptr = XLogCtl->lastFpwDisableRecPtr;
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	return recptr;
+}
+
+/*
+ * Get the lastBackupStar.
+ */
+XLogRecPtr
+GetlastBackupStart(bool need_lock)
+{
+	XLogRecPtr recptr;
+
+	if (need_lock)
+		WALInsertLockAcquireExclusive();
+
+	recptr = XLogCtl->Insert.lastBackupStart;
+
+	if (need_lock)
+		WALInsertLockRelease();
+
+	return recptr;
+}
+
+/*
+ * Set the lastBackupStar.
+ */
+void
+SetlastBackupStart(bool need_lock, XLogRecPtr recptr)
+{
+	if (need_lock)
+		WALInsertLockAcquireExclusive();
+
+	XLogCtl->Insert.lastBackupStart = recptr;
+
+	if (need_lock)
+		WALInsertLockRelease();
+}
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index dce265098e..8109209eae 100644
--- 

Re: [Commitfest 2022-09] Date is Over.

[Ibrar Ahmed]

On Wed, 5 Oct 2022 at 1:43 PM, Alvaro Herrera 
wrote:

> On 2022-Oct-03, Ibrar Ahmed wrote:
>
> > The date of the current commitfest is over, here is the current status of
> > the  "September 2022 commitfest."
> > There were 296 patches in the commitfest and 58 were get committed.
>
> Are you moving the open patches to the next commitfest, closing some as
> RwF, etc?  I'm not clear what the status is, for the November commitfest.


I am also not clear about that should I move that or wait till November.
Anybody guide me

>
>
> --
> Álvaro Herrera PostgreSQL Developer  —
> https://www.EnterpriseDB.com/
> "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
>
-- 

Ibrar Ahmed.
Senior Software Engineer, PostgreSQL Consultant.

Re: logical replication restrictions

[Peter Smith]

Hi Euler, a long time ago you ask me a few questions about my previous
review [1].

Here are my replies, plus a few other review comments for patch v7-0001.

==

1. doc/src/sgml/catalogs.sgml

+  
+   Application delay of changes by a specified amount of time. The
+   unit is in milliseconds.
+  

The wording sounds a bit strange still. How about below

SUGGESTION
The length of time (ms) to delay the application of changes.

===

2. Other documentation?

Maybe should say something on the Logical Replication Subscription
page about this? (31.2 Subscription)

===

3. doc/src/sgml/ref/create_subscription.sgml

+  synchronized, this may lead to apply changes earlier than expected.
+  This is not a major issue because a typical setting of this parameter
+  are much larger than typical time deviations between servers.

Wording?

SUGGESTION
... than expected, but this is not a major issue because this
parameter is typically much larger than the time deviations between
servers.

~~~

4. Q/A

>From [2] you asked:

> Should there also be a big warning box about the impact if using
> synchronous_commit (like the other streaming replication page has this
> warning)?
Impact? Could you elaborate?

~

I noticed the streaming replication docs for recovery_min_apply_delay
has a big red warning box saying that setting this GUC may block the
synchronous commits. So I was saying won’t a similar big red warning
be needed also for this min_apply_delay parameter if the delay is used
in conjunction with a publisher wanting synchronous commit because it
might block everything?

~~~

4. Example

+
+CREATE SUBSCRIPTION foo
+ CONNECTION 'host=192.168.1.50 port=5432 user=foo dbname=foodb'
+PUBLICATION baz
+   WITH (copy_data = false, min_apply_delay = '4h');
+

If the example named the subscription/publication as ‘mysub’ and
‘mypub’ I think it would be more consistent with the existing
examples.

==

5. src/backend/commands/subscriptioncmds.c - SubOpts

@@ -89,6 +91,7 @@ typedef struct SubOpts
  bool disableonerr;
  char*origin;
  XLogRecPtr lsn;
+ int64 min_apply_delay;
 } SubOpts;

I feel it would be better to be explicit about the storage units. So
call this member ‘min_apply_delay_ms’. E.g. then other code in
parse_subscription_options will be more natural when you are
converting using and assigning them to this member.

~~~

6. - parse_subscription_options

+ /*
+ * If there is no unit, interval_in takes second as unit. This
+ * parameter expects millisecond as unit so add a unit (ms) if
+ * there isn't one.
+ */

The comment feels awkward. How about below

SUGGESTION
If no unit was specified, then explicitly add 'ms' otherwise the
interval_in function would assume 'seconds'

~~~

7. - parse_subscription_options

(This is a repeat of [1] review comment #12)

+ if (opts->min_apply_delay < 0 && IsSet(supported_opts,
SUBOPT_MIN_APPLY_DELAY))
+ ereport(ERROR,
+ errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("option \"%s\" must not be negative", "min_apply_delay"));

Why is this code here instead of inside the previous code block where
the min_apply_delay was assigned in the first place?

==

8. src/backend/replication/logical/worker.c  - apply_delay

+ * When min_apply_delay parameter is set on subscriber, we wait long enough to
+ * make sure a transaction is applied at least that interval behind the
+ * publisher.

"on subscriber" -> "on the subscription"

~~~

9.

+ * Apply delay only after all tablesync workers have reached READY state. A
+ * tablesync worker are kept until it reaches READY state. If we allow the


Wording ??

"A tablesync worker are kept until it reaches READY state." ??

~~~

10.

10a.
+ /* nothing to do if no delay set */

Uppercase comment
/* Nothing to do if no delay set */

~

10b.
+ /* set apply delay */

Uppercase comment
/* Set apply delay */


~~~

11. - apply_handle_stream_prepare / apply_handle_stream_commit

The previous concern about incompatibility with the "Parallel Apply"
work (see [1] review comments #17, #18) is still a pending issue,
isn't it?

==

12. src/backend/utils/adt/timestamp.c interval_to_ms

+/*
+ * Given an Interval returns the number of milliseconds.
+ */
+int64
+interval_to_ms(const Interval *interval)

SUGGESTION
Returns the number of milliseconds in the specified Interval.

~~~

13.


+ /* adds portion time (in ms) to the previous result. */

Uppercase comment
/* Adds portion time (in ms) to the previous result. *

==

14. src/bin/pg_dump/pg_dump.c - getSubscriptions

+ {
+ appendPQExpBufferStr(query, " s.suborigin,\n");
+ appendPQExpBufferStr(query, " s.subapplydelay\n");
+ }

This could be done using just a single appendPQExpBufferStr if you
want to have 1 call instead of 2.

==

15. src/bin/psql/describe.c - describeSubscriptions

+ /* origin and min_apply_delay are only supported in v16 and higher */

Uppercase comment
/* Origin and min_apply_delay are only supported

Re: [PoC] Improve dead tuple storage for lazy vacuum

[John Naylor]

On Wed, Oct 5, 2022 at 1:46 PM Masahiko Sawada 
wrote:
>
> On Wed, Sep 28, 2022 at 12:49 PM Masahiko Sawada 
wrote:
> >
> > On Fri, Sep 23, 2022 at 12:11 AM John Naylor
> >  wrote:
> > Yeah, node31 and node256 are bloated.  We probably could use slab for
> > node256 independently. It's worth trying a benchmark to see how it
> > affects the performance and the tree size.

This wasn't the focus of your current email, but while experimenting with
v6 I had another thought about local allocation: If we use the default slab
block size of 8192 bytes, then only 3 chunks of size 2088 can fit, right?
If so, since aset and DSA also waste at least a few hundred bytes, we could
store a useless 256-byte slot array within node256. That way, node128 and
node256 share the same start of pointers/values array, so there would be
one less branch for getting that address. In v6, rt_node_get_values and
rt_node_get_children are not inlined (asde: gcc uses a jump table for 5
kinds but not for 4), but possibly should be, and the smaller the better.

> Regarding DSA support, IIUC we need to use dsa_pointer in inner nodes
> to point to its child nodes, instead of C pointers (ig, backend-local
> address). I'm thinking of a straightforward approach as the first
> step; inner nodes have a union of rt_node* and dsa_pointer and we
> choose either one based on whether the radix tree is shared or not. We
> allocate and free the shared memory for individual nodes by
> dsa_allocate() and dsa_free(), respectively. Therefore we need to get
> a C pointer from dsa_pointer by using dsa_get_address() while
> descending the tree. I'm a bit concerned that calling
> dsa_get_address() for every descent could be performance overhead but
> I'm going to measure it anyway.

Are dsa pointers aligned the same as pointers to locally allocated memory?
Meaning, is the offset portion always a multiple of 4 (or 8)? It seems that
way from a glance, but I can't say for sure. If the lower 2 bits of a DSA
pointer are never set, we can tag them the same way as a regular pointer.
That same technique could help hide the latency of converting the pointer,
by the same way it would hide the latency of loading parts of a node into
CPU registers.

One concern is, handling both local and dsa cases in the same code requires
more (predictable) branches and reduces code density. That might be a
reason in favor of templating to handle each case in its own translation
unit. But that might be overkill.
--
John Naylor
EDB: http://www.enterprisedb.com

Re: [Commitfest 2022-09] Date is Over.

[Alvaro Herrera]

On 2022-Oct-03, Ibrar Ahmed wrote:

> The date of the current commitfest is over, here is the current status of
> the  "September 2022 commitfest."
> There were 296 patches in the commitfest and 58 were get committed.

Are you moving the open patches to the next commitfest, closing some as
RwF, etc?  I'm not clear what the status is, for the November commitfest.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

Re: document the need to analyze partitioned tables

[Laurenz Albe]

On Mon, 2022-03-28 at 15:05 +0200, Tomas Vondra wrote:
> I've pushed the last version, and backpatched it to 10 (not sure I'd
> call it a bugfix, but I certainly agree with Justin it's worth
> mentioning in the docs, even on older branches).

I'd like to suggest an improvement to this.  The current wording could
be read to mean that dead tuples won't get cleaned up in partitioned tables.


By the way, where are the statistics of a partitioned tables used?  The actual
tables scanned are always the partitions, and in the execution plans that
I have seen, the optimizer always used the statistics of the partitions.

Yours,
Laurenz Albe
From 5209f228f09e52780535edacfee5f7efd2c25081 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Wed, 5 Oct 2022 10:31:47 +0200
Subject: [PATCH] Improve autovacuum doc on partitioned tables

The documentation mentioned that autovacuum doesn't process
partitioned tables, but it was unclear about the impact.
The old wording could be interpreted to mean that there are
problems with dead tuple cleanup on partitioned tables.
Clarify that the only potential problem is autoanalyze, and
that statistics for the partitions will be gathered.
---
 doc/src/sgml/maintenance.sgml | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml
index 759ea5ac9c..53e3fadbaf 100644
--- a/doc/src/sgml/maintenance.sgml
+++ b/doc/src/sgml/maintenance.sgml
@@ -860,10 +860,15 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu

 

-Partitioned tables are not processed by autovacuum.  Statistics
-should be collected by running a manual ANALYZE when it is
-first populated, and again whenever the distribution of data in its
-partitions changes significantly.
+Partitioned tables are not processed by autovacuum.  This is no problem
+as far as VACUUM is concerned, since autovacuum will process
+the partitions.  But, as mentioned in ,
+it also means that autovacuum won't run ANALYZE on the
+partitioned table itself.  While statistics are gathered for the partitions,
+some queries may rely on the statistics for the partitioned table.  You should
+collect statistics by running a manual ANALYZE when the
+partitioned table is first populated, and again whenever the distribution
+of data in its partitions changes significantly.

 

-- 
2.37.3

Add meson.build to version_stamp.pl

[Peter Eisentraut]

Thinking ahead a bit, we need to add meson.build to version_stamp.pl.

Maybe someone can think of a better sed expression, but this one seems 
good enough to me for now.From aae1dc51287655af3f847c3b7d07050f6e8651ec Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 5 Oct 2022 10:32:34 +0200
Subject: [PATCH] Add meson.build to version_stamp.pl

---
 src/tools/version_stamp.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/tools/version_stamp.pl b/src/tools/version_stamp.pl
index 9d4f44d4a95b..a8b696b28eba 100755
--- a/src/tools/version_stamp.pl
+++ b/src/tools/version_stamp.pl
@@ -95,6 +95,9 @@
 sed_file("configure.ac",
"-e 's/AC_INIT(\\[PostgreSQL\\], \\[[0-9a-z.]*\\]/AC_INIT([PostgreSQL], 
[$fullversion]/'"
 );
+sed_file("meson.build",
+   qq{-e "1,20s/ version: '[0-9a-z.]*',/ version: '$fullversion',/"}
+);
 
 print "Stamped these files with version number $fullversion:\n$fixedfiles";
 print "Don't forget to run autoconf $aconfver before committing.\n";
-- 
2.37.3

Re: [RFC] building postgres with meson - v13

[Peter Eisentraut]

On 04.10.22 05:25, Andres Freund wrote:

I've attached a revised version of the xml-tools dependency wrapper (0001):
Cleanups, minor error handling improvements, and bit of comment polishing. I'd
welcome review. But as it fixes a build-dependency bug / FIXME, I'm planning
to push it relatively soon otherwise.

0002 fixes libpq's .pc file (static dependencies didn't show up anymore) and
AIX compilation. AIX doesn't yet support link_whole (support was merged into
meson yesterday though). On the way it also improves comments and a bit of
generic infrastructure.  The price for now is that the static libpq is built
separately from the shared one, not reusing any objects. I felt that the
complexity of reusing the objects isn't worth it for now.

Peter, it'd be great if you could have a look at 0002.

0003 mirrors the setup of libpq to the various ecpg libraries. This is a
prerequisite to adding resource files.

0004 adds the resource files


These patches look ok to me.

Re: shadow variables - pg15 edition

[David Rowley]

On Tue, 4 Oct 2022 at 15:30, Justin Pryzby  wrote:
> Here, you forgot to change "val < 0".

Thanks. I made another review pass of each change to ensure I didn't
miss any others.  There were no other issues, so I pushed the adjusted
patch.

5 warnings remain.  4 of these are for PG_TRY() and co.

David

Re: Suppressing useless wakeups in walreceiver

[Alvaro Herrera]

On 2022-Oct-04, Nathan Bossart wrote:

> Here is an updated patch set with the following changes:
> 
> * The creation of the struct for non-shared WAL receiver state is moved to
> a prerequisite 0001 patch.  This should help ease review of 0002 a bit.

I think that would be even better if you moved the API adjustments of
some functions for the new struct to 0001 as well
(XLogWalRcvSendHSFeedback etc).

> * I updated the nap time calculation to round up to the next millisecond,
> as discussed upthread.

I didn't look at this part very carefully, but IIRC walreceiver's
responsivity has a direct impact on latency for sync replicas.  Maybe
it'd be sensible to the definition that the sleep time is rounded down
rather than up?  (At least, for the cases where we have something to do
and not merely continue sleeping.) 

> * I attempted to minimize the calls to GetCurrentTimestamp().  The WAL
> receiver code already calls this function pretty liberally, so I don't know
> if this is important, but perhaps it could make a difference for systems
> that don't have something like the vDSO to avoid real system calls.

Yeah, we are indeed careful about this in most places.  Maybe it's not
particularly important in 2022 and also not particularly important for
walreceiver (again, except in the code paths that affect sync replicas),
but there's no reason not to continue to be careful until we discover
more widely that it doesn't matter.

> * I removed the 'tli' argument from functions that now have an argument for
> the non-shared state struct.  The 'tli' is stored within the struct, so the
> extra argument is unnecessary.

+1  (This could also be in 0001, naturally.)

> One thing that still bugs me a little bit about 0002 is that the calls to
> GetCurrentTimestamp() feel a bit scattered and more difficult to reason
> about.  AFAICT 0002 keeps 'now' relatively up-to-date, but it seems
> possible that a future change could easily disrupt that.  I don't have any
> other ideas at the moment, though.

Hmm.

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

Re: Move backup-related code to xlogbackup.c/.h

[Michael Paquier]

On Tue, Oct 04, 2022 at 03:54:20PM -0700, Nathan Bossart wrote:
> I would suggest moving this to a separate prerequisite patch that can be
> reviewed independently from the patches that simply move code to a
> different file.

And FWIW, the SQL interfaces for pg_backup_start() and
pg_backup_stop() could stay in xlogfuncs.c.  This has the advantage to
centralize in the same file all the SQL-function-specific checks.
--
Michael


signature.asc
Description: PGP signature

Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths

[Michael Paquier]

Hi Matthias,

On Wed, Jul 27, 2022 at 02:07:05PM +0200, Matthias van de Meent wrote:

My apologies for the time it took me to come back to this thread.
> > + * To accommodate some overhead, hhis MaxXLogRecordSize value allows for
> > s/hhis/this/.
> 
> Will be included in the next update..

v8 fails to apply.  Could you send a rebased version?

As far as I recall the problems with the block image sizes are solved,
but we still have a bit more to do in terms of the overall record
size.  Perhaps there are some parts of the patch you'd like to
revisit?

For now, I have switched the back as waiting on author, and moved it
to the next CF.
--
Michael


signature.asc
Description: PGP signature

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

[Michael Paquier]

On Tue, Sep 20, 2022 at 01:33:09PM +0200, Drouvot, Bertrand wrote:
> I assume (maybe i should not) that if objects starting with / already exist
> there is very good reason(s) behind. Then I don't think that preventing
> their creation in the DDL would help (quite the contrary for the ones that
> really need them).

I have been pondering on this point for the last few weeks, and I'd
like to change my opinion and side with Tom on this one as per the
very unlikeliness of this being a problem in the wild.  I have studied
the places that would require restrictions but that was just feeling
adding a bit more bloat into the CREATE/ALTER ROLE paths for what's
aimed at providing a consistent experience for the user across
pg_hba.conf and pg_ident.conf.

> It looks to me that adding a GUC (off by default) to enable/disable the
> regexp usage in the hba could be a fair compromise. It won't block any
> creation starting with a / and won't open more doors (if such objects exist)
> by default.

Enforcing a behavior change in HBA policies with a GUC does not strike
me as a good thing in the long term.  I am ready to bet that it would
just sit around for nothing like the compatibility GUCs.

Anyway, I have looked at the patch.

+   List   *roles_re;
+   List   *databases_re;
+   regex_thostname_re;
I am surprised by the approach of using separate lists for the regular
expressions and the raw names.  Wouldn't it be better to store
everything in a single list but assign an entry type?  In this case it 
would be either regex or plain string.  This would minimize the
footprint of the changes (no extra arguments *_re in the routines
checking for a match on the roles, databases or hosts).  And it seems
to me that this would make unnecessary the use of re_num here and
there.  The hostname is different, of course, requiring only an extra
field for its type, or something like that.

Perhaps the documentation would gain in clarity if there were more
examples, like a set of comma-separated examples (mix of regex and raw
strings for example, for all the field types that gain support for
regexes)?

-$node->append_conf('postgresql.conf', "log_connections = on\n");
+$node->append_conf(
+'postgresql.conf', qq{
+listen_addresses = '127.0.0.1'
+log_connections = on
+});
Hmm.  I think that we may need to reconsider the location of the tests
for the regexes with the host name, as the "safe" regression tests
should not switch listen_addresses.  One location where we already do
that is src/test/ssl/, so these could be moved there.  Keeping the
database and user name parts in src/test/authentication/ is fine.

Something that stood out on a first review is the refactoring of
001_password.pl that can be done independently of the main patch:
- test_role() -> test_conn() to be able to pass down a database name.
- reset_pg_hba() to control the host, db and user parts.  The host
part does not really apply after moving the hosts checks to a more
secure location, so I guess that this had better be extended just for
the user and database, keeping host=local all the time.
I am planning to apply 0001 attached independently, reducing the
footprint of 0002, which is your previous patch left untouched
(mostly!).
--
Michael
From 203122ff1eeae942fc78b5d7ad85122547a2a6ed Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 5 Oct 2022 15:14:41 +0900
Subject: [PATCH v4 1/2] Refactor TAP test 001_password.pl

---
 src/test/authentication/t/001_password.pl | 60 ---
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/src/test/authentication/t/001_password.pl 
b/src/test/authentication/t/001_password.pl
index 58e4176e80..9e02697355 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -24,28 +24,30 @@ if (!$use_unix_sockets)
 sub reset_pg_hba
 {
my $node   = shift;
+   my $host   = shift;
+   my $database   = shift;
+   my $role   = shift;
my $hba_method = shift;
 
unlink($node->data_dir . '/pg_hba.conf');
# just for testing purposes, use a continuation line
-   $node->append_conf('pg_hba.conf', "local all all\\\n $hba_method");
+   $node->append_conf('pg_hba.conf', "$host $database $role\\\n 
$hba_method");
$node->reload;
return;
 }
 
-# Test access for a single role, useful to wrap all tests into one.  Extra
-# named parameters are passed to connect_ok/fails as-is.
-sub test_role
+# Test access for a connection string, useful to wrap all tests into one.
+# Extra named parameters are passed to connect_ok/fails as-is.
+sub test_conn
 {
local $Test::Builder::Level = $Test::Builder::Level + 1;
 
-   my ($node, $role, $method, $expected_res, %params) = @_;
+   my ($node, $connstr, $method, $expected_res, %params) = @_;
my $status_string = 'failed';
$status_string = 'success' if ($expected_res eq 0);
 
-   my $connstr = "user=$role";
my $testn