Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

[David Geier]

Hi Lukas,

On 1/2/23 20:50, Lukas Fittl wrote:
Thanks for continuing to work on this patch, and my apologies for 
silence on the patch.


It would be great if you could review it.
Please also share your thoughts around exposing the used clock source as 
GUC and renaming INSTR_TIME_GET_DOUBLE() to _SECS().


I rebased again on master because of [1]. Patches attached.



Its been hard to make time, and especially so because I typically 
develop on an ARM-based macOS system where I can't test this directly 
- hence my tests with virtualized EC2 instances, where I ran into the 
timing oddities.
That's good and bad. Bad to do the development and good to test the 
implementation on more virtualized setups; given that I also encountered 
"interesting" behavior on VMWare (see my previous mails).


On Mon, Jan 2, 2023 at 5:28 AM David Geier  wrote:

The INSTR_TIME_GET_MICROSEC() returns a uint64 while the other
variants
return double. This seems error prone. What about renaming the
function
or also have the function return a double and cast where necessary at
the call site?


Minor note, but in my understanding using a uint64 (where we can) is 
faster for any simple arithmetic we do with the values.


That's true. So the argument could be that for seconds and milliseconds 
we want the extra precision while microseconds are precise enough. 
Still, we could also make the seconds and milliseconds conversion code 
integer only and e.g. return two integers with the value before and 
after the comma. FWICS, the functions are nowhere used in performance 
critical code, so it doesn't really make a difference performance-wise.




+1, and feel free to carry this patch forward - I'll try to make an 
effort to review my earlier testing issues again, as well as your 
later improvements to the patch.

Moved to the current commit fest. Will you become reviewer?


Also, FYI, I just posted an alternate idea for speeding up EXPLAIN 
ANALYZE with timing over in [0], using a sampling-based approach to 
reduce the timing overhead.


Interesting idea. I'll reply with some thoughts on the corresponding thread.

[1] 
https://www.postgresql.org/message-id/flat/CALDaNm3kRBGPhndujr9JcjjbDCG3anhj0vW8b9YtbXrBDMSvvw%40mail.gmail.com


--
David Geier
(ServiceNow)
From f63527c8e4b3b0b71ffacaa111dd93325d973432 Mon Sep 17 00:00:00 2001
From: David Geier 
Date: Thu, 17 Nov 2022 10:22:01 +0100
Subject: [PATCH 1/3] Change instr_time to just store nanoseconds, that's
 cheaper.

---
 src/include/portability/instr_time.h | 62 
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h
index 9ea1a68bd9..c64f21eb53 100644
--- a/src/include/portability/instr_time.h
+++ b/src/include/portability/instr_time.h
@@ -80,63 +80,53 @@
 #define PG_INSTR_CLOCK	CLOCK_REALTIME
 #endif
 
-typedef struct timespec instr_time;
+typedef int64 instr_time;
+#define NS_PER_S INT64CONST(10)
+#define US_PER_S INT64CONST(100)
+#define MS_PER_S INT64CONST(1000)
 
-#define INSTR_TIME_IS_ZERO(t)	((t).tv_nsec == 0 && (t).tv_sec == 0)
+#define NS_PER_MS INT64CONST(100)
+#define NS_PER_US INT64CONST(1000)
 
-#define INSTR_TIME_SET_ZERO(t)	((t).tv_sec = 0, (t).tv_nsec = 0)
+#define INSTR_TIME_IS_ZERO(t)	((t) == 0)
 
-#define INSTR_TIME_SET_CURRENT(t)	((void) clock_gettime(PG_INSTR_CLOCK, &(t)))
+#define INSTR_TIME_SET_ZERO(t)	((t) = 0)
+
+static inline instr_time pg_clock_gettime_ns(void)
+{
+	struct timespec tmp;
+
+	clock_gettime(PG_INSTR_CLOCK, &tmp);
+
+	return tmp.tv_sec * NS_PER_S + tmp.tv_nsec;
+}
+
+#define INSTR_TIME_SET_CURRENT(t) \
+	(t) = pg_clock_gettime_ns()
 
 #define INSTR_TIME_ADD(x,y) \
 	do { \
-		(x).tv_sec += (y).tv_sec; \
-		(x).tv_nsec += (y).tv_nsec; \
-		/* Normalize */ \
-		while ((x).tv_nsec >= 10) \
-		{ \
-			(x).tv_nsec -= 10; \
-			(x).tv_sec++; \
-		} \
+		(x) += (y); \
 	} while (0)
 
 #define INSTR_TIME_SUBTRACT(x,y) \
 	do { \
-		(x).tv_sec -= (y).tv_sec; \
-		(x).tv_nsec -= (y).tv_nsec; \
-		/* Normalize */ \
-		while ((x).tv_nsec < 0) \
-		{ \
-			(x).tv_nsec += 10; \
-			(x).tv_sec--; \
-		} \
+		(x) -= (y); \
 	} while (0)
 
 #define INSTR_TIME_ACCUM_DIFF(x,y,z) \
 	do { \
-		(x).tv_sec += (y).tv_sec - (z).tv_sec; \
-		(x).tv_nsec += (y).tv_nsec - (z).tv_nsec; \
-		/* Normalize after each add to avoid overflow/underflow of tv_nsec */ \
-		while ((x).tv_nsec < 0) \
-		{ \
-			(x).tv_nsec += 10; \
-			(x).tv_sec--; \
-		} \
-		while ((x).tv_nsec >= 10) \
-		{ \
-			(x).tv_nsec -= 10; \
-			(x).tv_sec++; \
-		} \
+		(x) += (y) - (z); \
 	} while (0)
 
 #define INSTR_TIME_GET_DOUBLE(t) \
-	(((double) (t).tv_sec) + ((double) (t).tv_nsec) / 10.0)
+	((double) (t) / NS_PER_S)
 
 #define INSTR_TIME_GET_MILLISEC(t) \
-	(((double) (t).tv_sec * 1000.0) + ((double) (t).tv_nsec) / 100.0)
+	((double) (t) / NS_PER_MS)
 
 #define INSTR_TIME_GET_MICROSEC(t) 

Re: typos

[Michael Paquier]

On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> One minor comment:
> -spoken in Belgium (BE), with a UTF-8 character set
> +spoken in Belgium (BE), with a UTF-8 character set
> 
> Shouldn't this be UTF8 as we are using in func.sgml?

Yeah, I was wondering as well why this change is not worse, which is
why I left it out of 33ab0a2.  There is an acronym for UTF in
acronym.sgml, which makes sense to me, but that's the only place where 
this is used.  To add more on top of that, the docs basically need
only UTF8, and we have three references to UTF-16, none of them using
the  markup.
--
Michael


signature.asc
Description: PGP signature

Re: Data loss on logical replication, 12.12 to 14.5, ALTER SUBSCRIPTION

[Michail Nikolaev]

Hello, Amid.

> The point which is not completely clear from your description is the
> timing of missing records. In one of your previous emails, you seem to
> have indicated that the data missed from Table B is from the time when
> the initial sync for Table B was in-progress, right? Also, from your
> description, it seems there is no error or restart that happened
> during the time of initial sync for Table B. Is that understanding
> correct?

Yes and yes.
* B sync started - 08:08:34
* lost records are created - 09:49:xx
* B initial sync finished - 10:19:08
* I/O error with WAL - 10:19:22
* SIGTERM - 10:35:20

"Finished" here is `logical replication table synchronization worker
for subscription "cloud_production_main_sub_v4", table "B" has
finished`.
As far as I know, it is about COPY command.

> I am not able to see how these steps can lead to the problem.

One idea I have here - it is something related to the patch about
forbidding of canceling queries while waiting for synchronous
replication acknowledgement [1].
It is applied to Postgres in the cloud we were using [2]. We started
to see such errors in 10:24:18:

  `The COMMIT record has already flushed to WAL locally and might
not have been replicated to the standby. We must wait here.`

I wonder could it be some tricky race because of downtime of
synchronous replica and queries stuck waiting for ACK forever?

> If the problem is reproducible at your end, you might want to increase LOG
> verbosity to DEBUG1 and see if there is additional information in the
> LOGs that can help or it would be really good if there is a
> self-sufficient test to reproduce it.

Unfortunately, it looks like it is really hard to reproduce.

Best regards,
Michail.

[1]: 
https://www.postgresql.org/message-id/flat/CALj2ACU%3DnzEb_dEfoLqez5CLcwvx1GhkdfYRNX%2BA4NDRbjYdBg%40mail.gmail.com#8b7ffc8cdecb89de43c0701b4b6b5142
[2]: 
https://www.postgresql.org/message-id/flat/CAAhFRxgcBy-UCvyJ1ZZ1UKf4Owrx4J2X1F4tN_FD%3Dfh5wZgdkw%40mail.gmail.com#9c71a85cb6009eb60d0361de82772a50

Re: Allow placeholders in ALTER ROLE w/o superuser

[Pavel Borisov]

On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov  wrote:
>
> On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > I'm going to push this if no objections.
> >
> > I also suggest that meson.build should not copy regress_args.
>
> Good point, thanks.
Nice, thanks!
Isn't there the same reason to remove regress_args from meson.build in
oat's test and possibly from other modules with runningcheck=false?

Regards,
Pavel Borisov

Re: Perform streaming logical transactions by background workers and parallel apply

[shveta malik]

On Tue, Jan 3, 2023 at 11:10 AM wangw.f...@fujitsu.com
 wrote:
>
> On Mon, Jan 2, 2023 at 18:54 PM Amit Kapila  wrote:
> > On Fri, Dec 30, 2022 at 3:55 PM wangw.f...@fujitsu.com
> >  wrote:
> > >
> > > I've checked it and it looks good to me.
> > > Rebased the other patches and ran the pgident for the patch set.
> > >
> > > Attach the new patch set.
> > >
> >
> > I have added a few DEBUG messages and changed a few comments in the
> > 0001 patch. With that v71-0001* looks good to me and I'll commit it
> > later this week (by Thursday or Friday) unless there are any major
> > comments or objections.
>
> Thanks for your improvement.
>
> Rebased the patch set because the new change in HEAD (c8e1ba7).
> Attach the new patch set.
>
> Regards,
> Wang wei

Hi,
In continuation with [1] and [2], I did some performance testing on
v70-0001 patch.

This test used synchronous logical replication and compared SQL
execution times before and after applying the patch.

The following cases are tested by varying logical_decoding_work_mem:
a) Bulk insert.
b) Bulk delete
c) Bulk update
b) Rollback to savepoint. (different percentages of changes in the
transaction are rolled back).

The tests are performed ten times, and the average of the middle eight is taken.

The scripts are the same as before [1]. The scripts for additional
update and delete testing are attached.

The results are as follows:

RESULT - bulk insert (5kk)
---
logical_decoding_work_mem 64kB256kB   64MB
HEAD 34.475  34.222  34.400
patched  20.168  20.181  20.510
Compare with HEAD-41.49% -41.029%-40.377%


RESULT - bulk delete (5kk)
---
logical_decoding_work_mem 64kB256kB   64MB
HEAD 40.286  41.312  41.312
patched  23.749  23.759  23.480
Compare with HEAD -41.04% -42.48%-43.16%


RESULT - bulk update (5kk)
---
logical_decoding_work_mem 64kB256kB   64MB
HEAD 63.650  65.260  65.459
patched  46.692  46.275  48.281
Compare with HEAD-26.64% -29.09%-26.24%


RESULT - rollback 10% (5kk)
---
logical_decoding_work_mem 64kB256kB   64MB
HEAD33.386  33.213  31.990
patched  20.540  19.295  18.139
Compare with HEAD -38.47% -41.90%-43.29%


RESULT - rollback 20% (5kk)
---
logical_decoding_work_mem 64kB256kB   64MB
HEAD 32.150  31.871  30.825
patched  19.331  19.366  18.285
Compare with HEAD-39.87% -39.23% -40.68%


RESULT - rollback 30% (5kk)
---
logical_decoding_work_mem   64kB256kB   64MB
HEAD  28.611  30.139  29.433
patched   19.632  19.838  18.374
Compare with HEAD   -31.38% -34.17%  -37.57%


RESULT - rollback 50% (5kk)
---
logical_decoding_work_mem   64kB256kB   64MB
HEAD   27.410  27.167 25.990
patched19.982  18.749 18.048
Compare with HEAD   -27.099%-30.98%  -30.55%

(if "Compare with HEAD" is a positive number, it means worse than
HEAD; if it is a negative number, it means better than HEAD.)

Summary:
Update shows 26-29% improvement, while insert and delete shows ~40% improvement.
In the case of rollback, the improvement is somewhat between 27-42%.
The improvement slightly decreases with larger amounts of data being
rolled back.


[1] 
https://www.postgresql.org/message-id/OSZPR01MB63103AA97349BBB858E27DEAFD499%40OSZPR01MB6310.jpnprd01.prod.outlook.com
[2] 
https://www.postgresql.org/message-id/OSZPR01MB6310174063C9144D2081F657FDE09%40OSZPR01MB6310.jpnprd01.prod.outlook.com

thanks
Shveta
<>

Re: An oversight in ExecInitAgg for grouping sets

[Richard Guo]

On Tue, Jan 3, 2023 at 5:25 AM Tom Lane  wrote:

> Agreed, that's a latent bug.  It's only latent because the word just
> before a palloc chunk will never be zero, either in our historical
> palloc code or in v16's shiny new implementation.  Nonetheless it
> is a horrible idea for ExecInitAgg to depend on that fact, so I
> pushed your fix.


Thanks for pushing it!


> The thing that I find really distressing here is that it's been
> like this for years and none of our automated testing caught it.
> You'd have expected valgrind testing to do so ... but it does not,
> because we've never marked that word NOACCESS.  Maybe we should
> rethink that?  It'd require making mcxt.c do some valgrind flag
> manipulations so it could access the hdrmask when appropriate.


Yeah, maybe we can do that.  It's true that it requires some additional
work to access hdrmask, as in the new implementation the palloc'd chunk
is always prefixed by hdrmask.

BTW, I noticed a typo in the comment of memorychunk.h.

--- a/src/include/utils/memutils_memorychunk.h
+++ b/src/include/utils/memutils_memorychunk.h
@@ -5,9 +5,9 @@
  *   MemoryContexts may use as a header for chunks of memory they
allocate.
  *
  * MemoryChunk provides a lightweight header that a MemoryContext can use
to
- * store a reference back to the block the which the given chunk is
allocated
- * on and also an additional 30-bits to store another value such as the
size
- * of the allocated chunk.
+ * store a reference back to the block which the given chunk is allocated
on
+ * and also an additional 30-bits to store another value such as the size
of
+ * the allocated chunk.

Thanks
Richard

Re: Simplify standby state machine a bit in WaitForWALToBecomeAvailable()

[Bharath Rupireddy]

On Tue, Jan 3, 2023 at 7:47 AM Michael Paquier  wrote:
>
> On Fri, Dec 30, 2022 at 10:32:57AM -0800, Nathan Bossart wrote:
> > This looks correct to me.  The only thing that stood out to me was the loop
> > through 'tles' in XLogFileReadyAnyTLI.  With this change, we'd loop through
> > the timelines for both XLOG_FROM_PG_ARCHIVE and XLOG_FROM_PG_WAL, whereas
> > now we only loop through the timelines once.  However, I doubt this makes
> > much difference in practice.  You'd only do the extra loop whenever
> > restoring from the archives failed.
>
> case XLOG_FROM_ARCHIVE:
> +
> +   /*
> +* After failing to read from archive, we try to read from
> +* pg_wal.
> +*/
> +   currentSource = XLOG_FROM_PG_WAL;
> +   break;
> In standby mode, the priority lookup order is pg_wal -> archive ->
> stream.  With this change, we would do pg_wal -> archive -> pg_wal ->
> stream, meaning that it could influence some recovery scenarios while
> involving more lookups than necessary to the local pg_wal/ directory?
>
> See, on failure where the current source is XLOG_FROM_ARCHIVE, we
> would not switch anymore directly to XLOG_FROM_STREAM.

I think there's a bit of disconnect here - here's what I understand:

Standby when started can either enter to crash recovery (if it is a
restart after crash) or enter to archive recovery directly.

The standby, when in crash recovery:
currentSource is set to XLOG_FROM_PG_WAL in
WaitForWALToBecomeAvailable() and it continues to exhaust replaying
all the WAL in the pg_wal directory.
After all the pg_wal is exhausted during crash recovery, currentSource
is set to XLOG_FROM_ANY in ReadRecord() and the standby enters archive
recovery mode (see below).

The standby, when in archive recovery:
In WaitForWALToBecomeAvailable() currentSource is set to
XLOG_FROM_ARCHIVE and it enters XLogFileReadAnyTLI() - first tries to
fetch WAL from archive and returns if succeeds otherwise tries to
fetch from pg_wal and returns if succeeds, otherwise returns with
failure.
If failure is returned from XLogFileReadAnyTLI(), change the
currentSource to XLOG_FROM_STREAM.
If a failure in XLOG_FROM_STREAM, the currentSource is set to
XLOG_FROM_ARCHIVE and XLogFileReadAnyTLI() is called again.

Note that the standby exits from this WaitForWALToBecomeAvailable()
state machine when the promotion signal is detected and before which
all the wal from archive -> pg_wal is exhausted.

Note that currentSource is set to XLOG_FROM_PG_WAL in
WaitForWALToBecomeAvailable() only after the server exits archive
recovery i.e. InArchiveRecovery is set to false in
FinishWalRecovery(). However, exhausting pg_wal for recovery is built
inherently within XLogFileReadAnyTLI().

In summary:
the flow when the standby is in crash recovery is pg_wal -> [archive
-> pg_wal -> stream] -> [archive -> pg_wal -> stream] -> [] -> [] ...
the flow when the standby is in archive recovery is [archive -> pg_wal
-> stream] -> [archive -> pg_wal -> stream] -> [] -> [] ...

The proposed patch makes the inherent state change to pg_wal after
failure to read from archive in XLogFileReadAnyTLI() to explicit by
setting currentSource to XLOG_FROM_PG_WAL in the state machine. I
think it doesn't alter the existing state machine or add any new extra
lookups in pg_wal.

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

Re: Simplify standby state machine a bit in WaitForWALToBecomeAvailable()

[Bharath Rupireddy]

On Sat, Dec 31, 2022 at 12:03 AM Nathan Bossart
 wrote:
>
> On Tue, Oct 18, 2022 at 12:01:07PM +0530, Bharath Rupireddy wrote:
> > The attached patch attempts to simplify the code a bit by changing the
> > current source to XLOG_FROM_PG_WAL after failing in
> > XLOG_FROM_PG_ARCHIVE so that the state machine can move smoothly to
> > read from pg_wal. And we can just pass the current source to
> > XLogFileReadAnyTLI(). It also enables us to reduce a bit of extra
> > XLogFileRead() code in XLogFileReadAnyTLI().
>
> This looks correct to me.  The only thing that stood out to me was the loop
> through 'tles' in XLogFileReadyAnyTLI.  With this change, we'd loop through
> the timelines for both XLOG_FROM_PG_ARCHIVE and XLOG_FROM_PG_WAL, whereas
> now we only loop through the timelines once.  However, I doubt this makes
> much difference in practice.  You'd only do the extra loop whenever
> restoring from the archives failed.

Right. With the patch, we'd loop again through the tles after a
failure from the archive. Since the curFileTLI isn't changed unless a
successful read, we'd read from pg_wal with tli where we earlier left
off reading from the archive. I'm not sure if this extra looping is
worth worrying about.

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

128-bit integers can range only up to (2 ^ 63 -1)

[jian he]

include/pg_config.h
14: #define ALIGNOF_PG_INT128_TYPE 16
355: #define MAXIMUM_ALIGNOF 8
374: #define PG_INT128_TYPE __int128

/include/c.h
507: /*
508:  * 128-bit signed and unsigned integers
509:  * There currently is only limited support for such types.
510:  * E.g. 128bit literals and snprintf are not supported; but math is.
511:  * Also, because we exclude such types when choosing MAXIMUM_ALIGNOF,
512:  * it must be possible to coerce the compiler to allocate them on no
513:  * more than MAXALIGN boundaries.
514:  */
515: #if defined(PG_INT128_TYPE)
516: #if defined(pg_attribute_aligned) || ALIGNOF_PG_INT128_TYPE <=
MAXIMUM_ALIGNOF
517: #define HAVE_INT128 1
518:
519: typedef PG_INT128_TYPE int128
520: #if defined(pg_attribute_aligned)
521: pg_attribute_aligned(MAXIMUM_ALIGNOF)
522: #endif
523:   ;
524:
525: typedef unsigned PG_INT128_TYPE uint128
526: #if defined(pg_attribute_aligned)
527: pg_attribute_aligned(MAXIMUM_ALIGNOF)
528: #endif
529:   ;
530:
531: #endif
532: #endif
533:


Hi.
I am slightly confused by the int128 type. I thought the 128 bit integer
means range type will be upto 2 ^ 127 - 1.
Now just copy the above code and test the int128 range.
int128 can only up to  9223372036854775807 (2 ^ 63 -1).

also
File: /home/jian/helloc/pg/pg_interval/include/pg_config_ext.h
6: /* Define to the name of a signed 64-bit integer type. */
7: #define PG_INT64_TYPE long int
I also thought  that 64-bit means range up to 2 ^ 63 -1. Obviously I was
wrong.

So when we say "128 bit" what does it actually mean?

-- 
 I recommend David Deutsch's <>

  Jian

Re: TAP output format in pg_regress

[vignesh C]

On Tue, 29 Nov 2022 at 00:57, Daniel Gustafsson  wrote:
>
> > On 28 Nov 2022, at 20:02, Nikolay Shaplov  wrote:
>
> > From my reviewer's point of view patch is ready for commit.
> >
> > Thank you for your patience :-)
>
> Thanks for review.
>
> The attached tweaks a few comments and attempts to address the compiler 
> warning
> error in the CFBot CI.  Not sure I entirely agree with the compiler there but
> here is an attempt to work around it at least (by always copying the va_list
> for the logfile). It also adds a missing va_end for the logfile va_list.
>
> I hope this is close to a final version of this patch (commitmessage needs a
> bit of work though).
>
> > PS Should I change commitfest status?
>
> Sure, go ahead.
>

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch
./v14-0001-Change-pg_regress-output-format-to-be-TAP-compli.patch
patching file meson.build
Hunk #1 FAILED at 2968.
1 out of 1 hunk FAILED -- saving rejects to file meson.build.rej

[1] - http://cfbot.cputube.org/patch_41_3837.log

Regards,
Vignesh

Re: Add BufFileRead variants with short read and EOF detection

[vignesh C]

On Wed, 28 Dec 2022 at 16:17, Peter Eisentraut
 wrote:
>
> Most callers of BufFileRead() want to check whether they read the full
> specified length.  Checking this at every call site is very tedious.
> This patch provides additional variants BufFileReadExact() and
> BufFileReadMaybeEOF() that include the length checks.
>
> I considered changing BufFileRead() itself, but this function is also
> used in extensions, and so changing the behavior like this would create
> a lot of problems there.  The new names are analogous to the existing
> LogicalTapeReadExact().

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch
./0001-Add-BufFileRead-variants-with-short-read-and-EOF-det.patch
patching file src/backend/access/gist/gistbuildbuffers.c
...
Hunk #1 FAILED at 38.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/storage/buffile.h.rej

[1] - http://cfbot.cputube.org/patch_41_4089.log

Regards,
Vignesh

Re: Add SHELL_EXIT_CODE to psql

[vignesh C]

On Wed, 21 Dec 2022 at 11:04, Corey Huinker  wrote:
>
> I've rebased and updated the patch to include documentation.
>
> Regression tests have been moved to a separate patchfile because error 
> messages will vary by OS and configuration, so we probably can't do a stable 
> regression test, but having them handy at least demonstrates the feature.
>
> On Sun, Dec 4, 2022 at 12:35 AM Corey Huinker  wrote:
>>
>> Rebased. Still waiting on feedback before working on documentation.
>>
>> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker  wrote:
>>>
>>> Oops, that sample output was from a previous run, should have been:
>>>
>>> -- SHELL_EXIT_CODE is undefined
>>> \echo :SHELL_EXIT_CODE
>>> :SHELL_EXIT_CODE
>>> -- bad \!
>>> \! borp
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- bad backtick
>>> \set var `borp`
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- good \!
>>> \! true
>>> \echo :SHELL_EXIT_CODE
>>> 0
>>> -- play with exit codes
>>> \! exit 4
>>> \echo :SHELL_EXIT_CODE
>>> 4
>>> \set var `exit 3`
>>> \echo :SHELL_EXIT_CODE
>>> 3
>>>
>>>
>>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker  
>>> wrote:


 Over in 
 https://www.postgresql.org/message-id/eaf326ad693e74eba068f33a7f518...@oss.nttdata.com
  Justin Pryzby suggested that psql might need the ability to capture the 
 shell exit code.

 This is a POC patch that does that, but doesn't touch on the ON_ERROR_STOP 
 stuff.

 I've added some very rudimentary tests, but haven't touched the 
 documentation, because I strongly suspect that someone will suggest a 
 better name for the variable.

 But basically, it works like this

 -- SHELL_EXIT_CODE is undefined
 \echo :SHELL_EXIT_CODE
 :SHELL_EXIT_CODE
 -- bad \!
 \! borp
 sh: line 1: borp: command not found
 \echo :SHELL_EXIT_CODE
 32512
 -- bad backtick
 \set var `borp`
 sh: line 1: borp: command not found
 \echo :SHELL_EXIT_CODE
 127
 -- good \!
 \! true
 \echo :SHELL_EXIT_CODE
 0
 -- play with exit codes
 \! exit 4
 \echo :SHELL_EXIT_CODE
 1024
 \set var `exit 3`
 \echo :SHELL_EXIT_CODE
 3


 Feedback welcome.

CFBot shows some compilation errors as in [1], please post an updated
version for the same:
[02:35:49.924] psqlscanslash.l: In function ‘evaluate_backtick’:
[02:35:49.924] psqlscanslash.l:822:11: error: implicit declaration of
function ‘WIFSTOPPED’ [-Werror=implicit-function-declaration]
[02:35:49.924] 822 | exit_code=WSTOPSIG(exit_code);
[02:35:49.924] | ^~
[02:35:49.924] psqlscanslash.l:823:14: error: implicit declaration of
function ‘WSTOPSIG’ [-Werror=implicit-function-declaration]
[02:35:49.924] 823 | }
[02:35:49.924] | ^
[02:35:49.924] cc1: all warnings being treated as errors
[02:35:49.925] make[3]: *** [: psqlscanslash.o] Error 1

[1] - https://cirrus-ci.com/task/5424476720988160

Regards,
Vignesh

Re: [Proposal] Add foreign-server health checks infrastructure

[vignesh C]

On Tue, 20 Dec 2022 at 07:22, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear hackers,
>
> PSA rebased patches.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch ./v21-0003-add-test.patch
patching file contrib/postgres_fdw/expected/postgres_fdw.out
Hunk #1 FAILED at 11701.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/postgres_fdw/expected/postgres_fdw.out.rej
patching file contrib/postgres_fdw/expected/postgres_fdw_1.out
patching file contrib/postgres_fdw/sql/postgres_fdw.sql
Hunk #1 FAILED at 3876.
1 out of 1 hunk FAILED -- saving rejects to file
contrib/postgres_fdw/sql/postgres_fdw.sql.rej

[1] - http://cfbot.cputube.org/patch_41_3388.log

Regards,
Vignesh

Re: Add last_vacuum_index_scans in pg_stat_all_tables

[vignesh C]

On Tue, 4 Oct 2022 at 10:20, Ken Kato  wrote:
>
> > The problem is that you're not closing the 
>
> Thank you for the reviews and comments.
> I closed the  so that the problem should be fixed now.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./show_index_scans_in_pg_stat_all_tables_v3.patch
patching file src/backend/utils/activity/pgstat_relation.c
Hunk #1 succeeded at 209 (offset 1 line).
Hunk #2 FAILED at 232.
1 out of 2 hunks FAILED -- saving rejects to file
src/backend/utils/activity/pgstat_relation.c.rej
patching file src/include/pgstat.h
Hunk #1 FAILED at 366.
1 out of 2 hunks FAILED -- saving rejects to file src/include/pgstat.h.rej
patching file src/test/regress/expected/rules.out
Hunk #1 succeeded at 1800 (offset 8 lines).
Hunk #2 succeeded at 2145 (offset 11 lines).
Hunk #3 succeeded at 2193 (offset 14 lines).

[1] - http://cfbot.cputube.org/patch_41_3756.log

Regards,
Vignesh

Re: Allow placeholders in ALTER ROLE w/o superuser

[Alexander Korotkov]

On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov  wrote:
> On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov  wrote:
> >
> > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > > I'm going to push this if no objections.
> > >
> > > I also suggest that meson.build should not copy regress_args.
> >
> > Good point, thanks.
> Nice, thanks!
> Isn't there the same reason to remove regress_args from meson.build in
> oat's test and possibly from other modules with runningcheck=false?

This makes sense to me too.  I don't see anything specific in oat's
regression test that requires setting regress_args.

--
Regards,
Alexander Korotkov

Re: Add index scan progress to pg_stat_progress_vacuum

[vignesh C]

On Mon, 2 Jan 2023 at 10:04, Imseih (AWS), Sami  wrote:
>
> >cfbot is complaining that this patch no longer applies.  Sami, would you
> >mind rebasing it?
>
> Rebased patch attached.

CFBot shows some compilation errors as in [1], please post an updated
version for the same:
[07:01:58.889] In file included from ../../../src/include/postgres.h:47,
[07:01:58.889] from vacuumparallel.c:27:
[07:01:58.889] vacuumparallel.c: In function ‘parallel_vacuum_update_progress’:
[07:01:58.889] vacuumparallel.c:1118:10: error: ‘IsParallelWorker’
undeclared (first use in this function); did you mean
‘ParallelWorkerMain’?
[07:01:58.889] 1118 | Assert(!IsParallelWorker);
[07:01:58.889] | ^~~~
[07:01:58.889] ../../../src/include/c.h:859:9: note: in definition of
macro ‘Assert’
[07:01:58.889] 859 | if (!(condition)) \
[07:01:58.889] | ^
[07:01:58.889] vacuumparallel.c:1118:10: note: each undeclared
identifier is reported only once for each function it appears in
[07:01:58.889] 1118 | Assert(!IsParallelWorker);
[07:01:58.889] | ^~~~

[1] - https://cirrus-ci.com/task/4557389261701120

Regards,
Vignesh

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[vignesh C]

On Fri, 9 Dec 2022 at 20:41, Reid Thompson
 wrote:
>
> On Tue, 2022-12-06 at 10:32 -0800, Andres Freund wrote:
> > Hi,
> >
> > On 2022-11-26 22:22:15 -0500, Reid Thompson wrote:
> > > rebased/patched to current master && current pg-stat-activity-
> > > backend-memory-allocated
> >
> > This version fails to build with msvc, and builds with warnings on
> > other
> > platforms.
> > https://cirrus-ci.com/build/5410696721072128
> > msvc:
> >
> > Andres Freund
>
> updated patches

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch
./0001-Add-tracking-of-backend-memory-allocated-to-pg_stat_.patch
...
patching file src/backend/utils/mmgr/slab.c
Hunk #1 succeeded at 69 (offset 16 lines).
Hunk #2 succeeded at 414 (offset 175 lines).
Hunk #3 succeeded at 436 with fuzz 2 (offset 176 lines).
Hunk #4 FAILED at 286.
Hunk #5 succeeded at 488 (offset 186 lines).
Hunk #6 FAILED at 381.
Hunk #7 FAILED at 554.
3 out of 7 hunks FAILED -- saving rejects to file
src/backend/utils/mmgr/slab.c.rej

[1] - http://cfbot.cputube.org/patch_41_3867.log

Regards,
Vignesh

Re: Add tracking of backend memory allocated to pg_stat_activity

[vignesh C]

On Thu, 8 Dec 2022 at 19:44, Reid Thompson
 wrote:
>
> On Sun, 2022-11-27 at 09:40 -0600, Justin Pryzby wrote:
> > > BTW, these should have some kind of prefix, like PG_ALLOC_* to
> > > avoid causing the same kind of problem for someone else that
> > > another header caused for you by defining something somewhere
> > > called IGNORE (ignore what, I don't know).  The other problem was
> > > probably due to a define, though.  Maybe instead of an enum, the
> > > function should take a boolean.
> > >
>
> Patch updated to current master and includes above prefix
> recommendation and combining of two function calls to one recommended
> by Ted Yu.
>
> > >
> > > I still wonder whether there needs to be a separate CF entry for
> > > the 0001 patch.  One issue is that there's two different lists of
> > > people involved in the threads.
> > >
>
> I'm OK with containing the conversation to one thread if everyone else
> is.  If there's no argument against, then patches after today will go
> to the "Add the ability to limit the amount of memory that can be
> allocated to backends" thread
> https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch
./0001-Add-tracking-of-backend-memory-allocated-to-pg_stat_.patch
patching file src/backend/utils/mmgr/slab.c
Hunk #1 succeeded at 69 (offset 16 lines).
Hunk #2 succeeded at 414 (offset 175 lines).
Hunk #3 succeeded at 436 with fuzz 2 (offset 176 lines).
Hunk #4 FAILED at 286.
Hunk #5 succeeded at 488 (offset 186 lines).
Hunk #6 FAILED at 381.
Hunk #7 FAILED at 554.
3 out of 7 hunks FAILED -- saving rejects to file
src/backend/utils/mmgr/slab.c.rej

[1] - http://cfbot.cputube.org/patch_41_3865.log

Regards,
Vignesh

Re: Question about initial logical decoding snapshot

[Amit Kapila]

On Fri, Dec 30, 2022 at 11:57 PM Chong Wang  wrote:
>
> I'm studying the source code about creation of initial logical decoding 
> snapshot. What confused me is that why must we process 3 xl_running_xacts 
> before we get to the consistent state. I think we only need 2 
> xl_running_xacts.
>
> I think we can get to consistent state when we meet the 2nd xl_running_xact 
> with its oldestRunningXid > 1st xl_running_xact's nextXid, this means the 
> active transactions in 1st xl_running_xact all had commited, and we have all 
> the logs of transactions who will commit afterwards, so there is consistent 
> state in this time point and we can export a snapshot.
>

Yeah, we will have logs for all transactions in such a case but I
think we won't have a valid snapshot by that time. Consider a case
that there are two transactions 723,724 in the 2nd xl_running_xact
record for which we have waited to finish and then consider that point
as a consistent point and exported that snapshot. It is quite possible
that by that time the commit record of one or more of those xacts (say
724) wouldn't have been encountered by decoding process and that means
it won't be recorded in the xip list of the snapshot (we do that in
DecodeCommit->SnapBuildCommitTxn). So, during export in function
SnapBuildInitialSnapshot(), we will consider 723 as committed and 724
as running. This could not lead to inconsistent data on the client
side that imports such a snapshot and use it for copy and further
replicating the other xacts.

OTOH, currently, before marking snapshot state as consistent we wait
for these xacts to finish and for another xl_running_xact where
oldestRunningXid >= builder->next_phase_at to appear which means the
commit for both 723 and 724 would have appeared in the snapshot.

Does that makes sense to you or am, I missing something here?

-- 
With Regards,
Amit Kapila.

Re: Question about initial logical decoding snapshot

[Amit Kapila]

On Tue, Jan 3, 2023 at 4:44 PM Amit Kapila  wrote:
>
> On Fri, Dec 30, 2022 at 11:57 PM Chong Wang  wrote:
> >
> > I'm studying the source code about creation of initial logical decoding 
> > snapshot. What confused me is that why must we process 3 xl_running_xacts 
> > before we get to the consistent state. I think we only need 2 
> > xl_running_xacts.
> >
> > I think we can get to consistent state when we meet the 2nd xl_running_xact 
> > with its oldestRunningXid > 1st xl_running_xact's nextXid, this means the 
> > active transactions in 1st xl_running_xact all had commited, and we have 
> > all the logs of transactions who will commit afterwards, so there is 
> > consistent state in this time point and we can export a snapshot.
> >
>
> Yeah, we will have logs for all transactions in such a case but I
> think we won't have a valid snapshot by that time. Consider a case
> that there are two transactions 723,724 in the 2nd xl_running_xact
> record for which we have waited to finish and then consider that point
> as a consistent point and exported that snapshot. It is quite possible
> that by that time the commit record of one or more of those xacts (say
> 724) wouldn't have been encountered by decoding process and that means
> it won't be recorded in the xip list of the snapshot (we do that in
> DecodeCommit->SnapBuildCommitTxn). So, during export in function
> SnapBuildInitialSnapshot(), we will consider 723 as committed and 724
> as running. This could not lead to inconsistent data on the client
> side that imports such a snapshot and use it for copy and further
> replicating the other xacts.
>
> OTOH, currently, before marking snapshot state as consistent we wait
> for these xacts to finish and for another xl_running_xact where
> oldestRunningXid >= builder->next_phase_at to appear which means the
> commit for both 723 and 724 would have appeared in the snapshot.
>
> Does that makes sense to you or am, I missing something here?
>

You can also refer to the discussion in the thread [1] which is
related to your question.

[1] - 
https://www.postgresql.org/message-id/c94be044-818f-15e3-1ad3-7a7ae2dfed0a%40iki.fi

-- 
With Regards,
Amit Kapila.

Re: Allow placeholders in ALTER ROLE w/o superuser

[Pavel Borisov]

Hi, Alexander!

On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov  wrote:
>
> On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov  wrote:
> > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov  
> > wrote:
> > >
> > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > > > I'm going to push this if no objections.
> > > >
> > > > I also suggest that meson.build should not copy regress_args.
> > >
> > > Good point, thanks.
> > Nice, thanks!
> > Isn't there the same reason to remove regress_args from meson.build in
> > oat's test and possibly from other modules with runningcheck=false?
>
> This makes sense to me too.  I don't see anything specific in oat's
> regression test that requires setting regress_args.
Yes, it seems so.
Regress args in oat's Makefile are added as a response to a buildfarm
issues by 7c51b7f7cc08. They seem unneeded to be copied into
meson.build with runningcheck=false. I may mistake but it seems to me
that removing regress_args from meson.build with runningcheck=false is
just to make it neat, not for functionality. So I consider fixing it
in pg_db_role_setting, oat, or both of them optional.

Regards,
Pavel Borisov.

Re: Time delayed LR (WAS Re: logical replication restrictions)

[shveta malik]

>
> On Tue, 27 Dec 2022 at 14:59, Hayato Kuroda (Fujitsu)
>  wrote:
> > Note that more than half of the modifications are done by Osumi-san.
>

Please find a few minor comments.
1.
+  diffms = TimestampDifferenceMilliseconds(GetCurrentTimestamp(),
+

   TimestampTzPlusMilliseconds(ts, MySubscription->minapplydelay));
 on unix, above code looks unaligned (copied from unix)

2. same with:
+  interval = DatumGetIntervalP(DirectFunctionCall3(interval_in,
+

   CStringGetDatum(val),
+

   ObjectIdGetDatum(InvalidOid),
+

   Int32GetDatum(-1)));
perhaps due to tabs?

2. comment not clear:
+  * During the time delayed replication, avoid reporting the suspended
+  * latest LSN are already flushed and written, to the publisher.

3.
+  * Call send_feedback() to prevent the publisher from exiting by
+  * timeout during the delay, when wal_receiver_status_interval is
+  * available. The WALs for this delayed transaction is neither
+  * written nor flushed yet, Thus, we don't make the latest LSN
+  * overwrite those positions of the update message for this delay.

 yet, Thus, we -->  yet, thus, we/   yet. Thus, we


4.
+  /* Adds portion time (in ms) to the previous result. */
+  ms = interval->time / INT64CONST(1000);
Is interval->time always in micro-seconds here?



Thanks
Shveta

Re: Data loss on logical replication, 12.12 to 14.5, ALTER SUBSCRIPTION

[Amit Kapila]

On Tue, Jan 3, 2023 at 2:14 PM Michail Nikolaev
 wrote:
>
> > The point which is not completely clear from your description is the
> > timing of missing records. In one of your previous emails, you seem to
> > have indicated that the data missed from Table B is from the time when
> > the initial sync for Table B was in-progress, right? Also, from your
> > description, it seems there is no error or restart that happened
> > during the time of initial sync for Table B. Is that understanding
> > correct?
>
> Yes and yes.
> * B sync started - 08:08:34
> * lost records are created - 09:49:xx
> * B initial sync finished - 10:19:08
> * I/O error with WAL - 10:19:22
> * SIGTERM - 10:35:20
>
> "Finished" here is `logical replication table synchronization worker
> for subscription "cloud_production_main_sub_v4", table "B" has
> finished`.
> As far as I know, it is about COPY command.
>
> > I am not able to see how these steps can lead to the problem.
>
> One idea I have here - it is something related to the patch about
> forbidding of canceling queries while waiting for synchronous
> replication acknowledgement [1].
> It is applied to Postgres in the cloud we were using [2]. We started
> to see such errors in 10:24:18:
>
>   `The COMMIT record has already flushed to WAL locally and might
> not have been replicated to the standby. We must wait here.`
>

Does that by any chance mean you are using a non-community version of
Postgres which has some other changes?

> I wonder could it be some tricky race because of downtime of
> synchronous replica and queries stuck waiting for ACK forever?
>

It is possible but ideally, in that case, the client should request
such a transaction again.

-- 
With Regards,
Amit Kapila.

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[vignesh C]

On Tue, 20 Dec 2022 at 04:27, Corey Huinker  wrote:
>
>
> Attached is my work in progress to implement the changes to the CAST() 
> function as proposed by Vik Fearing.
>
> This work builds upon the Error-safe User Functions work currently ongoing.
>
> The proposed changes are as follows:
>
> CAST(expr AS typename)
> continues to behave as before.
>
> CAST(expr AS typename ERROR ON ERROR)
> has the identical behavior as the unadorned CAST() above.
>
> CAST(expr AS typename NULL ON ERROR)
> will use error-safe functions to do the cast of expr, and will return 
> NULL if the cast fails.
>
> CAST(expr AS typename DEFAULT expr2 ON ERROR)
> will use error-safe functions to do the cast of expr, and will return 
> expr2 if the cast fails.
>
> There is an additional FORMAT parameter that I have not yet implemented, my 
> understanding is that it is largely intended for DATE/TIME field conversions, 
> but others are certainly possible.
> CAST(expr AS typename FORMAT fmt DEFAULT expr2 ON ERROR)
>
> What is currently working:
> - Any scalar expression that can be evaluated at parse time. These tests from 
> cast.sql all currently work:
>
> VALUES (CAST('error' AS integer));
> VALUES (CAST('error' AS integer ERROR ON ERROR));
> VALUES (CAST('error' AS integer NULL ON ERROR));
> VALUES (CAST('error' AS integer DEFAULT 42 ON ERROR));
>
> SELECT CAST('{123,abc,456}' AS integer[] DEFAULT '{-789}' ON ERROR) as 
> array_test1;
>
> - Scalar values evaluated at runtime.
>
> CREATE TEMPORARY TABLE t(t text);
> INSERT INTO t VALUES ('a'), ('1'), ('b'), (2);
> SELECT CAST(t.t AS integer DEFAULT -1 ON ERROR) AS foo FROM t;
>  foo
> -
>   -1
>1
>   -1
>2
> (4 rows)
>
>
> Along the way, I made a few design decisions, each of which is up for debate:
>
> First, I created OidInputFunctionCallSafe, which is to OidInputFunctionCall 
> what InputFunctionCallSafe is to InputFunctionCall. Given that the only place 
> I ended up using it was stringTypeDatumSafe(), it may be possible to just 
> move that code inside stringTypeDatumSafe.
>
> Next, I had a need for FuncExpr, CoerceViaIO, and ArrayCoerce to all report 
> if their expr argument failed, and if not, just past the evaluation of expr2. 
> Rather than duplicate this logic in several places, I chose instead to modify 
> CoalesceExpr to allow for an error-test mode in addition to its default 
> null-test mode, and then to provide this altered node with two expressions, 
> the first being the error-safe typecast of expr and the second being the 
> non-error-safe typecast of expr2.
>
> I still don't have array-to-array casts working, as the changed I would 
> likely need to make to ArrayCoerce get somewhat invasive, so this seemed like 
> a good time to post my work so far and solicit some feedback beyond what I've 
> already been getting from Jeff Davis and Michael Paquier.
>
> I've sidestepped domains as well for the time being as well as avoiding JIT 
> issues entirely.
>
> No documentation is currently prepared. All but one of the regression test 
> queries work, the one that is currently failing is:
>
> SELECT CAST('{234,def,567}'::text[] AS integer[] DEFAULT '{-1011}' ON ERROR) 
> as array_test2;
>
> Other quirks:
> - an unaliased CAST ON DEFAULT will return the column name of "coalesce", 
> which internally is true, but obviously would be quite confusing to a user.
>
> As a side observation, I noticed that the optimizer already tries to resolve 
> expressions based on constants and to collapse expression trees where 
> possible, which makes me wonder if the work done to do the same in 
> transformTypeCast/ and coerce_to_target_type and coerce_type isn't also 
> wasted.

CFBot shows some compilation errors as in [1], please post an updated
version for the same:
[02:53:44.829] time make -s -j${BUILD_JOBS} world-bin
[02:55:41.164] llvmjit_expr.c: In function ‘llvm_compile_expr’:
[02:55:41.164] llvmjit_expr.c:928:6: error: ‘v_resnull’ undeclared
(first use in this function); did you mean ‘v_resnullp’?
[02:55:41.164] 928 | v_resnull = LLVMBuildLoad(b, v_reserrorp, "");
[02:55:41.164] | ^
[02:55:41.164] | v_resnullp
[02:55:41.164] llvmjit_expr.c:928:6: note: each undeclared identifier
is reported only once for each function it appears in
[02:55:41.164] llvmjit_expr.c:928:35: error: ‘v_reserrorp’ undeclared
(first use in this function); did you mean ‘v_reserror’?
[02:55:41.164] 928 | v_resnull = LLVMBuildLoad(b, v_reserrorp, "");
[02:55:41.164] | ^~~
[02:55:41.164] | v_reserror
[02:55:41.165] make[2]: *** [: llvmjit_expr.o] Error 1
[02:55:41.165] make[2]: *** Waiting for unfinished jobs
[02:55:45.495] make[1]: *** [Makefile:42: all-backend/jit/llvm-recurse] Error 2
[02:55:45.495] make: *** [GNUmakefile:21: world-bin-src-recurse] Error 2

[1] - https://cirrus-ci.com/task/6687753371385856?logs=gcc_warning#L448

Regards,
Vignesh

Re: [PATCH] CF app: add "Returned: Needs more interest"

[vignesh C]

On Wed, 26 Oct 2022 at 04:25, Jacob Champion  wrote:
>
> On Mon, Aug 8, 2022 at 8:45 AM Andres Freund  wrote:
> > On 2022-08-08 08:37:41 -0700, Jacob Champion wrote:
> > > Agreed. This probably bleeds over into the other documentation thread
> > > a bit -- how do we want to communicate the subtle points to people in
> > > a CF?
> >
> > We should write a docs patch for it and then reference if from a bunch of
> > places. I started down that road a few years back [1] but unfortunately lost
> > steam.
>
> As we approach a new CF, I'm reminded of this patch again.
>
> Are there any concerns preventing a consensus here, that I can help
> with? I can draft the docs patch that Andres has suggested, if that's
> seen as a prerequisite.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./0001-Add-a-Returned-Needs-more-interest-close-code.patch
patching file 
pgcommitfest/commitfest/migrations/0006_alter_patchoncommitfest_status.py
can't find file to patch at input line 57
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--
|diff --git a/pgcommitfest/commitfest/models.py
b/pgcommitfest/commitfest/models.py
|index 28722f0..433eb4a 100644
|--- a/pgcommitfest/commitfest/models.py
|+++ b/pgcommitfest/commitfest/models.py
--
No file to patch.  Skipping patch.
3 out of 3 hunks ignored
can't find file to patch at input line 85

[1] - http://cfbot.cputube.org/patch_41_3991.log

Regards,
Vignesh

Re: CI and test improvements

[vignesh C]

On Fri, 30 Dec 2022 at 09:29, Thomas Munro  wrote:
>
> On Wed, Nov 23, 2022 at 11:57 AM Justin Pryzby  wrote:
> > [PATCH 02/10] cirrus/macos: switch to "macos_instance" / M1..
>
> Duelling patches.
>
> Bilal's patch[1] uses the matrix feature to run the tests on both
> Intel and ARM, which made sense when he proposed it, but according to
> Cirrus CI warnings, the Intel instances are about to go away.  So I
> think we just need your smaller change to switch the instance type.
>
> As for the pathname change, there is another place that knows where
> Homebrew lives, in ldap/001_auth.  Fixed in the attached.  That test
> just SKIPs if it can't find the binary, making it harder to notice.
> Standardising our approach here might make sense for a later patch.
> As for the kerberos test, Bilal's patch may well be a better idea (it
> adds MacPorts for one thing), and so I'll suggest rebasing that, but
> here I just wanted the minimum mechanical fix to avoid breaking on the
> 1st of Jan.
>
> I plan to push this soon if there are no objections.  Then discussion
> of Bilal's patch can continue.
>
> > [PATCH 03/10] cirrus/macos: update to macos ventura
>
> I don't know any reason not to push this one too, but it's not time critical.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./0001-ci-Change-macOS-builds-from-Intel-to-ARM.patch
patching file .cirrus.yml
Hunk #1 FAILED at 407.
Hunk #2 FAILED at 428.
Hunk #3 FAILED at 475.
3 out of 3 hunks FAILED -- saving rejects to file .cirrus.yml.rej
patching file src/test/kerberos/t/001_auth.pl
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED -- saving rejects to file
src/test/kerberos/t/001_auth.pl.rej
patching file src/test/ldap/t/001_auth.pl
Hunk #1 FAILED at 21.
1 out of 1 hunk FAILED -- saving rejects to file src/test/ldap/t/001_auth.pl.rej

[1] - http://cfbot.cputube.org/patch_41_3709.log

Regards,
Vigneh

Re: Cygwin cleanup

[vignesh C]

On Wed, 9 Nov 2022 at 06:34, Justin Pryzby  wrote:
>
> On Thu, Oct 20, 2022 at 10:40:40PM -0500, Justin Pryzby wrote:
> > On Thu, Aug 04, 2022 at 04:16:06PM +1200, Thomas Munro wrote:
> > > On Thu, Aug 4, 2022 at 3:38 PM Justin Pryzby  wrote:
> > > > [train wreck]
> > >
> > > Oh my, so I'm getting the impression we might actually be totally
> > > unstable on Cygwin.  Which surprises me because ... wait a minute ...
> > > lorikeet isn't even running most of the tests.  So... we don't really
> > > know the degree to which any of this works at all?
> >
> > Right.
> >
> > Maybe it's of limited interest, but ..
> >
> > This updates the patch to build and test with meson.
> > Which first requires patching some meson.builds.
> > I guess that's needed for some current BF members, too.
> > Unfortunately, ccache+PCH causes gcc to crash :(
>
> Resending with the 'only-if' line commented (doh).
> And some fixes to 001 as Andres pointed out by on other thread.

Is there still some work pending for this thread as Andres had
committed some part, if so, can you post an updated patch for the
same.

Regards,
Vignesh

Re: [PATCH] Expand character set for ltree labels

[vignesh C]

On Thu, 6 Oct 2022 at 03:35, Garen Torikian  wrote:
>
> 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

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./0002-Expand-character-set-for-ltree-labels.patch
patching file contrib/ltree/expected/ltree.out
patching file contrib/ltree/ltree.h
Hunk #2 FAILED at 126.
1 out of 2 hunks FAILED -- saving rejects to file contrib/ltree/ltree.h.rej

[1] - http://cfbot.cputube.org/patch_41_3929.log

Regards,
Vignesh

Re: [PATCH]Feature improvement for MERGE tab completion

[vignesh C]

On Wed, 21 Sept 2022 at 10:55, Fujii Masao  wrote:
>
>
>
> On 2022/09/21 0:51, Alvaro Herrera wrote:
> > The rules starting at line 4111 make me a bit nervous, since nowhere
> > we're restricting them to operating only on MERGE lines.  I don't think
> > it's a real problem since USING is not terribly common anyway.  Likewise
> > for the ones with WHEN [NOT] MATCHED.  I kinda wish we had a way to
> > search for stuff like "keyword MERGE appears earlier in the command",
> > but we don't have that.
>
> Yeah, I was thinking the same when updating the patch.
>
> How about adding something like PartialMatches() that checks whether
> the keywords are included in the input string or not? If so, we can restrict
> some tab-completion rules to operating only on MERGE, as follows. I attached
> the WIP patch (0002 patch) that introduces PartialMatches().
> Is this approach over-complicated? Thought?
>
> +   else if (PartialMatches("MERGE", "INTO", MatchAny, "USING") ||
> +PartialMatches("MERGE", "INTO", MatchAny, "AS", 
> MatchAny, "USING") ||
> +PartialMatches("MERGE", "INTO", MatchAny, MatchAny, 
> "USING"))
> +   {
> +   /* Complete MERGE INTO ... ON with target table attributes */
> +   if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
> +   COMPLETE_WITH_ATTR(prev4_wd);
> +   else if (TailMatches("INTO", MatchAny, "AS", MatchAny, 
> "USING", MatchAny, "AS", MatchAny, "ON"))
> +   COMPLETE_WITH_ATTR(prev8_wd);
> +   else if (TailMatches("INTO", MatchAny, MatchAny, "USING", 
> MatchAny, MatchAny, "ON"))
> +   COMPLETE_WITH_ATTR(prev6_wd);

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./v9-0001-psql-Improve-tab-completion-for-MERGE.patch
patching file src/bin/psql/tab-complete.c
Hunk #1 FAILED at 1669.
Hunk #2 FAILED at 3641.
Hunk #3 FAILED at 3660.
Hunk #4 FAILED at 4065.
4 out of 4 hunks FAILED -- saving rejects to file
src/bin/psql/tab-complete.c.rej

[1] - http://cfbot.cputube.org/patch_41_3890.log

Regards,
Vignesh

Re: MultiXact\SLRU buffers configuration

[vignesh C]

On Fri, 19 Aug 2022 at 21:18,  wrote:
>
> Andrey Borodin wrote 2022-08-18 06:35:
> >
> > I like the idea of one knob instead of one per each SLRU. Maybe we
> > even could deduce sane value from NBuffers? That would effectively
> > lead to 0 knobs :)
> >
> > Your patch have a prefix "v22-0006", does it mean there are 5 previous
> > steps of the patchset?
> >
> > Thank you!
> >
> >
> > Best regards, Andrey Borodin.
>
> Not sure it's possible to deduce from NBuffers only.
> slru_buffers_scale_shift looks like relief valve for systems with ultra
> scaled NBuffers.
>
> Regarding v22-0006 I just tried to choose index unique for this thread
> so now it's fixed to 0001 indexing.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

=== Applying patches on top of PostgreSQL commit ID
325bc54eed4ea0836a0bb715bb18342f0c1c668a ===
=== applying patch ./v23-0001-bucketed-SLRUs-simplified.patch
patching file src/include/miscadmin.h
...
patching file src/backend/utils/misc/guc.c
Hunk #1 FAILED at 32.
Hunk #2 FAILED at 2375.
2 out of 2 hunks FAILED -- saving rejects to file
src/backend/utils/misc/guc.c.rej

[1] - http://cfbot.cputube.org/patch_41_2627.log

Regards,
Vignesh

Re: SLRUs in the main buffer pool - Page Header definitions

[vignesh C]

On Fri, 16 Dec 2022 at 04:47, Bagga, Rishu  wrote:
> Rebased and updated a new patch addressing the critical section issue in
> RecordNewMultliXact.In GetNewMultiXactId, we now make our ReadBuffer
> calls before starting the critical section, but while holding the
> MultiXactGenLock, so we always fetch the correct buffers. We store them
> in an array that is accessed later in RecordNewMultiXact.
> This way we can keep the existing functionality of only holding the 
> MultiXactGenLock while reading in buffers, but can let go when we are writing,
> to preserve the existing concurrency paradigm.
> Let me know your thoughts on this approach.
>

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch ./slru_to_buffer_cache_with_page_headers_v3.patch
...
patching file src/include/catalog/catversion.h
Hunk #1 FAILED at 57.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/catalog/catversion.h.rej

[1] - http://cfbot.cputube.org/patch_41_3514.log

Regards,
Vignesh

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

[vignesh C]

On Thu, 8 Dec 2022 at 08:17, Amit Langote  wrote:
>
> On Wed, Dec 7, 2022 at 8:54 PM Amit Langote  wrote:
> > Per Alvaro's advice, forking this from [1].
> >
> > In that thread, Tom had asked if it wouldn't be better to find a new
> > place to put extraUpdatedCols [2] instead of RangeTblEntry, along with
> > the permission-checking fields are now no longer stored in
> > RangeTblEntry.
> >
> > In [3] of the same thread, I proposed to move it into a List of
> > Bitmapsets in ModifyTable, as implemented in the attached patch that I
> > had been posting to that thread.
> >
> > The latest version of that patch is attached herewith.
>
> Updated to replace a list_nth() with list_nth_node() and rewrote the
> commit message.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch
./v2-0001-Add-per-result-relation-extraUpdatedCols-to-Modif.patch
...
patching file src/backend/optimizer/util/inherit.c
Hunk #2 FAILED at 50.
Hunk #9 succeeded at 926 with fuzz 2 (offset -1 lines).
1 out of 9 hunks FAILED -- saving rejects to file
src/backend/optimizer/util/inherit.c.rej

[1] - http://cfbot.cputube.org/patch_41_4049.log

Regards,
Vignesh

Re: [PATCH] New [relation] option engine

[vignesh C]

On Sun, 20 Nov 2022 at 11:42, Nikolay Shaplov  wrote:
>
> В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay
> Shaplov написал:
>
> > > > > cfbot reports the patch no longer applies.  As CommitFest 2022-11 is
> > > > > currently underway, this would be an excellent time to update the
> > > > > patch.
> > > >
> > > > Oups! I should have done it before...
> > > > Fixed
> > >
> > > Trying to fix meson build
> >
> > Trying to fix compiler warnings.
>
> Patched rebased. Imported changes from 4f981df8 commit (Report a more useful
> error for reloptions on a partitioned table)

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch ./new_options_take_two_v03f.patch
patching file src/include/access/reloptions.h
Hunk #1 FAILED at 1.
1 out of 4 hunks FAILED -- saving rejects to file
src/include/access/reloptions.h.rej

[1] - http://cfbot.cputube.org/patch_41_3536.log

Regards,
Vignesh

Re: Optimizing Node Files Support

[vignesh C]

On Fri, 2 Dec 2022 at 19:06, Ranier Vilela  wrote:
>
> Hi, thanks for reviewing this.
>
> Em sex., 2 de dez. de 2022 às 09:24, John Naylor 
>  escreveu:
>>
>>
>> On Thu, Dec 1, 2022 at 8:02 PM Ranier Vilela  wrote:
>> >
>> > Hi,
>> >
>> > I believe that has room for improving generation node files.
>> >
>> > The patch attached reduced the size of generated files by 27 kbytes.
>> > From 891 kbytes to 864 kbytes.
>> >
>> > About the patch:
>> > 1. Avoid useless attribution when from->field is NULL, once that
>> > the new node is palloc0.
>> >
>> > 2. Avoid useless declaration variable Size, when it is unnecessary.
>>
>> Not useless -- it prevents a multiple evaluation hazard, which this patch 
>> introduces.
>
> It's doubting, that patch introduces some hazard here.
> But I think that casting size_t (typedef Size) to size_t is worse and is 
> unnecessary.
> Adjusted in the v1 patch.
>
>>
>> > 3. Optimize comparison functions like memcmp and strcmp, using
>> >  a short-cut comparison of the first element.
>>
>> Not sure if the juice is worth the squeeze. Profiling would tell.
>
> This is a cheaper test and IMO can really optimize, avoiding a function call.
>
>>
>> > 4. Switch several copy attributions like COPY_SCALAR_FIELD or 
>> > COPY_LOCATION_FIELD
>> > by one memcpy call.
>>
>> My first thought is, it would cause code churn.
>
> It's a weak argument.
> Reduced 27k from source code, really worth it.
>
>>
>> > 5. Avoid useless attribution, ignoring the result of pg_strtok when it is 
>> > unnecessary.
>>
>> Looks worse.
>
> Better to inform the compiler that we really don't need the result.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./v1-optimize_gen_nodes_support.patch
patching file src/backend/nodes/gen_node_support.pl
Hunk #2 succeeded at 680 with fuzz 2.
Hunk #3 FAILED at 709.
...
Hunk #7 succeeded at 844 (offset 42 lines).
1 out of 7 hunks FAILED -- saving rejects to file
src/backend/nodes/gen_node_support.pl.rej

[1] - http://cfbot.cputube.org/patch_41_4034.log

Regards,
Vignesh

Re: WIN32 pg_import_system_collations

[Peter Eisentraut]

On 09.12.22 13:48, Juan José Santamaría Flecha wrote:
On Thu, Dec 1, 2022 at 8:46 AM Peter Eisentraut 
> wrote:



What is the status of this now?  I think the other issue has been
addressed?


Yes, that's addressed for MSVC builds. I think there are a couple of 
pending issues for MinGW, but those should have their own threads.


The patch had rotten, so PFA a rebased version.


committed

Re: Todo: Teach planner to evaluate multiple windows in the optimal order

[Ankit Kumar Pandey]

Hi,

On 03/01/23 08:21, David Rowley wrote:


I do think you'll likely want to put any WindowClauses which have
pathkeys which are a true subset or true superset of the ORDER BY /
DISTINCT pathkeys last.  If they're a superset then we won't need to
perform any additional ordering for the DISTINCT / ORDER BY clause.
If they're a subset then we might be able to perform an Incremental
Sort, which is likely much cheaper than a full sort.  The existing
code should handle that part. You just need to make
select_active_windows() more intelligent.


I think current implementation does exactly this.

#1. If order by clause in the window function is subset of order by in query

create table abcd(a int, b int, c int, d int);
insert into abcd select x,y,z,c from generate_series(1,5) x, 
generate_Series(1,5)y, generate_Series(1,5) z, generate_Series(1,5) c;
explain analyze select a,row_number() over (order by b),count(*) over (order by 
a,b) from abcd order by a,b,c;

QUERY PLAN

--


 Incremental Sort  (cost=80.32..114.56 rows=625 width=28) (actual 
time=1.440..3.311 rows=625 loops=1)
   Sort Key: a, b, c
   Presorted Key: a, b
   Full-sort Groups: 13  Sort Method: quicksort  Average Memory: 28kB  Peak 
Memory: 28kB
   ->  WindowAgg  (cost=79.24..91.74 rows=625 width=28) (actual 
time=1.272..2.567 rows=625 loops=1)
 ->  Sort  (cost=79.24..80.80 rows=625 width=20) (actual 
time=1.233..1.296 rows=625 loops=1)
   Sort Key: a, b
   Sort Method: quicksort  Memory: 64kB
   ->  WindowAgg  (cost=39.27..50.21 rows=625 width=20) (actual 
time=0.304..0.786 rows=625 loops=1)
 ->  Sort  (cost=39.27..40.84 rows=625 width=12) (actual 
time=0.300..0.354 rows=625 loops=1)
   Sort Key: b
   Sort Method: quicksort  Memory: 54kB
   ->  Seq Scan on abcd  (cost=0.00..10.25 rows=625 
width=12) (actual time=0.021..0.161 rows=625 l
oops=1)
 Planning Time: 0.068 ms
 Execution Time: 3.509 ms
(15 rows)

Here, as window function (row count) has two cols a, b for order by, 
incremental sort is performed for remaining col in query,


which makes sense.


#2. If order by clause in the Window function is superset of order by in 
query


explain analyze select a,row_number() over (order by a,b,c),count(*) over 
(order by a,b) from abcd order by a;

  QUERY PLAN
--
 WindowAgg  (cost=39.27..64.27 rows=625 width=28) (actual time=1.089..3.020 
rows=625 loops=1)
   ->  WindowAgg  (cost=39.27..53.34 rows=625 width=20) (actual 
time=1.024..1.635 rows=625 loops=1)
 ->  Sort  (cost=39.27..40.84 rows=625 width=12) (actual 
time=1.019..1.084 rows=625 loops=1)
   Sort Key: a, b, c
   Sort Method: quicksort  Memory: 54kB
   ->  Seq Scan on abcd  (cost=0.00..10.25 rows=625 width=12) 
(actual time=0.023..0.265 rows=625 loops=1)
 Planning Time: 0.071 ms
 Execution Time: 3.156 ms
(8 rows)

No, additional sort is needed to be performed in this case, as you referred.

On 03/01/23 08:21, David Rowley wrote:
If they're a superset then we won't need to perform any additional 
ordering for the DISTINCT / ORDER BY clause.

If they're a subset then we might be able to perform an Incremental
Sort, which is likely much cheaper than a full sort.


So question is, how current implementation is different from desired one?

--
Regards,
Ankit Kumar Pandey

Re: Cygwin cleanup

[Justin Pryzby]

On Tue, Jan 03, 2023 at 05:54:56PM +0530, vignesh C wrote:
> > On Thu, Oct 20, 2022 at 10:40:40PM -0500, Justin Pryzby wrote:
> > > On Thu, Aug 04, 2022 at 04:16:06PM +1200, Thomas Munro wrote:
> > > > On Thu, Aug 4, 2022 at 3:38 PM Justin Pryzby  
> > > > wrote:
> > > > > [train wreck]
> > > >
> > > > Oh my, so I'm getting the impression we might actually be totally
> > > > unstable on Cygwin.  Which surprises me because ... wait a minute ...
> > > > lorikeet isn't even running most of the tests.  So... we don't really
> > > > know the degree to which any of this works at all?
> > >
> > > Right.
> > >
> > > Maybe it's of limited interest, but ..
> > >
> > > This updates the patch to build and test with meson.
> > > Which first requires patching some meson.builds.
> > > I guess that's needed for some current BF members, too.
> > > Unfortunately, ccache+PCH causes gcc to crash :(
> >
> > Resending with the 'only-if' line commented (doh).
> > And some fixes to 001 as Andres pointed out by on other thread.
> 
> Is there still some work pending for this thread as Andres had
> committed some part, if so, can you post an updated patch for the
> same.

Thomas, what's your opinion ?

Re: Allow placeholders in ALTER ROLE w/o superuser

[Justin Pryzby]

On Tue, Jan 03, 2023 at 02:20:38PM +0300, Pavel Borisov wrote:
> Hi, Alexander!
> 
> On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov  wrote:
> >
> > On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov  
> > wrote:
> > > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov  
> > > wrote:
> > > >
> > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  
> > > > wrote:
> > > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > > > > I'm going to push this if no objections.
> > > > >
> > > > > I also suggest that meson.build should not copy regress_args.
> > > >
> > > > Good point, thanks.
> > > Nice, thanks!
> > > Isn't there the same reason to remove regress_args from meson.build in
> > > oat's test and possibly from other modules with runningcheck=false?
> >
> > This makes sense to me too.  I don't see anything specific in oat's
> > regression test that requires setting regress_args.
> Yes, it seems so.
> Regress args in oat's Makefile are added as a response to a buildfarm
> issues by 7c51b7f7cc08. They seem unneeded to be copied into
> meson.build with runningcheck=false. I may mistake but it seems to me
> that removing regress_args from meson.build with runningcheck=false is
> just to make it neat, not for functionality. So I consider fixing it
> in pg_db_role_setting, oat, or both of them optional.

Right - my suggestion is to "uncopy" them from pg_db_role_setting, where
they serve no purpose, since they shouldn't have been copied originally.

On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote:
> On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > I'm going to push this if no objections.
> >
> > I also suggest that meson.build should not copy regress_args.
> 
> Good point, thanks.

I should've mentioned that the same things should be removed from
Makefile, too...

-- 
Justin

Re: Allow placeholders in ALTER ROLE w/o superuser

[Alexander Korotkov]

On Tue, Jan 3, 2023 at 5:28 PM Justin Pryzby  wrote:
> On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote:
> > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > > I also suggest that meson.build should not copy regress_args.
> >
> > Good point, thanks.
>
> I should've mentioned that the same things should be removed from
> Makefile, too...

This makes sense too.  See the attached patchset.

--
Regards,
Alexander Korotkov


0001-meson-Add-running-test-setup-as-a-replacement-for-v3.patch
Description: Binary data


0002-Remove-extra-regress-check-arguments-from-test_pg-v3.patch
Description: Binary data

Re: Allow placeholders in ALTER ROLE w/o superuser

[Pavel Borisov]

On Tue, 3 Jan 2023 at 17:28, Justin Pryzby  wrote:
>
> On Tue, Jan 03, 2023 at 02:20:38PM +0300, Pavel Borisov wrote:
> > Hi, Alexander!
> >
> > On Tue, 3 Jan 2023 at 13:48, Alexander Korotkov  
> > wrote:
> > >
> > > On Tue, Jan 3, 2023 at 11:51 AM Pavel Borisov  
> > > wrote:
> > > > On Tue, 3 Jan 2023 at 09:29, Alexander Korotkov  
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  
> > > > > wrote:
> > > > > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > > > > > I'm going to push this if no objections.
> > > > > >
> > > > > > I also suggest that meson.build should not copy regress_args.
> > > > >
> > > > > Good point, thanks.
> > > > Nice, thanks!
> > > > Isn't there the same reason to remove regress_args from meson.build in
> > > > oat's test and possibly from other modules with runningcheck=false?
> > >
> > > This makes sense to me too.  I don't see anything specific in oat's
> > > regression test that requires setting regress_args.
> > Yes, it seems so.
> > Regress args in oat's Makefile are added as a response to a buildfarm
> > issues by 7c51b7f7cc08. They seem unneeded to be copied into
> > meson.build with runningcheck=false. I may mistake but it seems to me
> > that removing regress_args from meson.build with runningcheck=false is
> > just to make it neat, not for functionality. So I consider fixing it
> > in pg_db_role_setting, oat, or both of them optional.
>
> Right - my suggestion is to "uncopy" them from pg_db_role_setting, where
> they serve no purpose, since they shouldn't have been copied originally.
>
> On Tue, Jan 03, 2023 at 09:29:00AM +0300, Alexander Korotkov wrote:
> > On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby  wrote:
> > > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote:
> > > > I'm going to push this if no objections.
> > >
> > > I also suggest that meson.build should not copy regress_args.
> >
> > Good point, thanks.
>
> I should've mentioned that the same things should be removed from
> Makefile, too...
>
> --
Thanks, Justin!
Attached is a new patch accordingly.

Regards,
Pavel Borisov!


v3-0001-meson-Add-running-test-setup-as-a-replacement-for.patch
Description: Binary data

Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication

[Melih Mutlu]

Hi hackers,

Sending an updated version of this patch to get rid of compiler warnings.

I would highly appreciate any feedback.

Thanks,
-- 
Melih Mutlu
Microsoft


v2-0001-Add-replication-protocol-cmd-to-create-a-snapshot.patch
Description: Binary data


v6-0002-Reuse-Logical-Replication-Background-worker.patch
Description: Binary data

Re: 128-bit integers can range only up to (2 ^ 63 -1)

[Tom Lane]

jian he  writes:
> I am slightly confused by the int128 type. I thought the 128 bit integer
> means range type will be upto 2 ^ 127 - 1.
> Now just copy the above code and test the int128 range.
> int128 can only up to  9223372036854775807 (2 ^ 63 -1).

What's your grounds for claiming that?

regards, tom lane

Re: Data loss on logical replication, 12.12 to 14.5, ALTER SUBSCRIPTION

[Michail Nikolaev]

> Does that by any chance mean you are using a non-community version of
> Postgres which has some other changes?

It is a managed Postgres service in the general cloud. Usually, such
providers apply some custom minor patches.
The only one I know about - about forbidding of canceling queries
while waiting for synchronous replication acknowledgement.

> It is possible but ideally, in that case, the client should request
> such a transaction again.

I am not sure I get you here.

I'll try to explain what I mean:

The patch I'm referring to does not allow canceling a query while it
waiting acknowledge for ACK for COMMIT message in case of synchronous
replication.
If synchronous standby is down - query and connection just stuck until
server restart (or until standby become available to process ACK).
Tuples changed by such a hanging transaction are not visible by other
transactions. It is all done to prevent seeing spurious tuples in case
of network split.

So, it seems like we had such a situation during that story because of
our synchronous standby downtime (before server restart).
My thoughts just about the possibility of fact that such transactions
(waiting for ACK for COMMIT) are handled somehow incorrectly by
logical replication engine.

Michail.

Re: Stack overflow issue

[Егор Чиндяскин]

>Среда, 26 октября 2022, 21:47 +07:00 от Egor Chindyaskin :
> 
>24.08.2022 20:58, Tom Lane writes:
>> Nice work! I wonder if you can make the regex crashes reachable by
>> reducing the value of max_stack_depth enough that it's hit before
>> reaching the "regular expression is too complex" limit.
>>
>> regards, tom lane Hello everyone! It's been a while since me and Alexander 
>> Lakhin have
>published a list of functions that have a stack overflow illness. We are
>back to tell you more about such places.
>During our analyze we made a conclusion that some functions can be
>crashed without changing any of the parameters and some can be crashed
>only if we change some stuff.
>
>The first function crashes without any changes:
>
># CheckAttributeType
>
>(n=6; printf "create domain dint as int; create domain dinta0 as
>dint[];"; for ((i=1;i<=$n;i++)); do printf "create domain dinta$i as
>dinta$(( $i - 1 ))[]; "; done; ) | psql
>psql -c "create table t(f1 dinta6[]);"
>
>Some of the others crash if we change "max_locks_per_transaction"
>parameter:
>
># findDependentObjects
>
>max_locks_per_transaction = 200
>
>(n=1; printf "create table t (i int); create view v0 as select *
>from t;"; for ((i=1;i<$n;i++)); do printf "create view v$i as select *
>from v$(( $i - 1 )); "; done; ) | psql
>psql -c "drop table t"
>
># ATExecDropColumn
>
>max_locks_per_transaction = 300
>
>(n=5; printf "create table t0 (a int, b int); "; for
>((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1
>))); "; done; printf "alter table t0 drop b;" ) | psql
>
># ATExecDropConstraint
>
>max_locks_per_transaction = 300
>
>(n=5; printf "create table t0 (a int, b int, constraint bc check (b
> > 0));"; for ((i=1;i<=$n;i++)); do printf "create table t$i()
>inherits(t$(( $i - 1 ))); "; done; printf "alter table t0 drop
>constraint bc;" ) | psql
>
># ATExecAddColumn
>
>max_locks_per_transaction = 200
>
>(n=5; printf "create table t0 (a int, b int);"; for
>((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1
>))); "; done; printf "alter table t0 add column c int;" ) | psql
>
># ATExecAlterConstrRecurse
>
>max_locks_per_transaction = 300
>
>(n=5;
>printf "create table t(a int primary key); create table pt (a int
>primary key, foreign key(a) references t) partition by range (a);";
>printf "create table pt0 partition of pt for values from (0) to (10)
>partition by range (a);";
>for ((i=1;i<=$n;i++)); do printf "create table pt$i partition of pt$((
>$i - 1 )) for values from ($i) to (10) partition by range (a); "; done;
>printf "alter table pt alter constraint pt_a_fkey deferrable initially
>deferred;"
>) | psql
>
>This is where the fun begins. According to Tom Lane, a decrease in
>max_stack_depth could lead to new crashes, but it turned out that
>Alexander was able to find new crashes precisely due to the increase in
>this parameter. Also, we had ulimit -s set to 8MB as the default value.
>
># eval_const_expressions_mutator
>
>max_stack_depth = '7000kB'
>
>(n=1; printf "select 'a' "; for ((i=1;i<$n;i++)); do printf "
>collate \"C\" "; done; ) | psql
>
>If you didn’t have a crash, like me, when Alexander shared his find,
>then probably you configured your cluster with an optimization flag -Og.
>In the process of trying to break this function, we came to the
>conclusion that the maximum stack depth depends on the optimization flag
>(-O0/-Og). As it turned out, when optimizing, the function frame on the
>stack becomes smaller and because of this, the limit is reached more
>slowly, therefore, the system can withstand more torment. Therefore,
>this query will fail if you have a cluster configured with the -O0
>optimization flag.
>
>The crash of the next function not only depends on the optimization
>flag, but also on a number of other things. While researching, we
>noticed that postgres enforces a distance ~400kB from max_stack_depth to
>ulimit -s. We thought we could hit the max_stack_depth limit and then
>hit the OS limit as well. Therefore, Alexander wrote a recursive SQL
>function, that eats up a stack within max_stack_depth, including a query
>that eats up the remaining ~400kB. And this causes a crash.
>
># executeBoolItem
>
>max_stack_depth = '7600kB'
>
>create function infinite_recurse(i int) returns int as $$
>begin
>   raise notice 'Level %', i;
>   begin
> perform jsonb_path_query('{"a":[1]}'::jsonb, ('$.a[*] ? (' ||
>repeat('!(', 4800) || '@ == @' || repeat(')', 4800) || ')')::jsonpath);
>   exception
> when others then raise notice 'jsonb_path_query error at level %,
>%', i, sqlerrm;
>   end;
>   begin
> select infinite_recurse(i + 1) into i;
>   exception
> when others then raise notice 'Max stack depth reached at level %,
>%', i, sqlerrm;
>   end;
>   return i;
>end;
>$$ language plpgsql;
>
>select infinite_recurse(1);
>
>To sum it all up, we have not yet decided on a general approach to such
>functions. Some functions are definitely subject to stack overflow

Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local

[Tom Lane]

Andres Freund  writes:
> On 2023-01-02 15:46:36 -0800, Andres Freund wrote:
>> I haven't seen any problems in HEAD, so I'm working on backpatching.

> And done.

It occurs to me that we should now be able to drop configure's
probe for -Wno-compound-token-split-by-macro, since that was only
needed to suppress warnings in the Perl headers.  Won't save much
of course, but every test we can get rid of is worth doing IMO.

regards, tom lane

Re: Is OpenSSL AES-NI not available in pgcrypto?

[Peter Eisentraut]

On 02.01.23 17:57, aghart...@gmail.com wrote:

select pgp_sym_encrypt(data::text, 'pwd') --default to aes128
from generate_series('2022-01-01'::timestamp, '2022-12-31'::timestamp, 
'1 hour'::interval) data


vs

select pgp_sym_encrypt(data::text, 'pwd','cipher-algo=bf') -- blowfish
from generate_series('2022-01-01'::timestamp, '2022-12-31'::timestamp, 
'1 hour'::interval) data


In my test both queries execution is similaraes-128 was expected 
about  5 time faster.


So, why?

Pgcrypto use OpenSSL as backend, so, does it explicit force software aes 
calculation instead of AES-NI cpu ones?


I suspect it is actually using AES hardware support, but all the other 
overhead of pgcrypto makes the difference not noticeable.

Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local

[Tom Lane]

I wrote:
> It occurs to me that we should now be able to drop configure's
> probe for -Wno-compound-token-split-by-macro, since that was only
> needed to suppress warnings in the Perl headers.

... or not.  A bit of experimentation says that they still come out,
apparently because the warnings are triggered by *use* of relevant
Perl macros not by their *definitions*.  Oh well.

regards, tom lane

Re: Is OpenSSL AES-NI not available in pgcrypto?

[aghart...@gmail.com]

Hi,

I see, I was hoping that wasn't the case.

Thanks a lot for your support.

My best regards,

Agharta


Il 03/01/23 16:54, Peter Eisentraut ha scritto:

On 02.01.23 17:57, aghart...@gmail.com wrote:

select pgp_sym_encrypt(data::text, 'pwd') --default to aes128
from generate_series('2022-01-01'::timestamp, 
'2022-12-31'::timestamp, '1 hour'::interval) data


vs

select pgp_sym_encrypt(data::text, 'pwd','cipher-algo=bf') -- blowfish
from generate_series('2022-01-01'::timestamp, 
'2022-12-31'::timestamp, '1 hour'::interval) data


In my test both queries execution is similaraes-128 was expected 
about  5 time faster.


So, why?

Pgcrypto use OpenSSL as backend, so, does it explicit force software 
aes calculation instead of AES-NI cpu ones?


I suspect it is actually using AES hardware support, but all the other 
overhead of pgcrypto makes the difference not noticeable.

Re: 128-bit integers can range only up to (2 ^ 63 -1)

[jian he]

On Tue, Jan 3, 2023 at 8:50 PM Tom Lane  wrote:

> jian he  writes:
> > I am slightly confused by the int128 type. I thought the 128 bit integer
> > means range type will be upto 2 ^ 127 - 1.
> > Now just copy the above code and test the int128 range.
> > int128 can only up to  9223372036854775807 (2 ^ 63 -1).
>
> What's your grounds for claiming that?
>
> regards, tom lane
>


I did something like int128 a1 = 9223372036854775807 +
1;
I also did something like int128 a1 = (int128)9223372036854775807000;
I misread the warning.  I should do the cast first.

The second expression has a warning. I guess because

> There is no support in GCC for expressing an integer constant of type
> __int128 for targets with long long integer less than 128 bits wide.
>
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fint128.html

Re: typos

[Peter Eisentraut]

On 03.01.23 09:41, Michael Paquier wrote:

On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:

One minor comment:
-spoken in Belgium (BE), with a UTF-8 character set
+spoken in Belgium (BE), with a UTF-8 character set

Shouldn't this be UTF8 as we are using in func.sgml?


Yeah, I was wondering as well why this change is not worse, which is
why I left it out of 33ab0a2.  There is an acronym for UTF in
acronym.sgml, which makes sense to me, but that's the only place where
this is used.  To add more on top of that, the docs basically need
only UTF8, and we have three references to UTF-16, none of them using
the  markup.


The thing is called "UTF-8".  Here, we are not talking about the 
PostgreSQL identifier.

Re: 128-bit integers can range only up to (2 ^ 63 -1)

[Tom Lane]

jian he  writes:
> On Tue, Jan 3, 2023 at 8:50 PM Tom Lane  wrote:
>> What's your grounds for claiming that?

> I did something like int128 a1 = 9223372036854775807 +
> 1;

Well, that's going to do the arithmetic in (probably) long int.

regards, tom lane

Re: [PATCH] CF app: add "Returned: Needs more interest"

[Jacob Champion]

On Tue, Jan 3, 2023 at 4:14 AM vignesh C  wrote:
> The patch does not apply on top of HEAD as in [1], please post a rebased 
> patch:

Hi Vignesh -- this is a patch for the CF app, not the Postgres repo,
so cfbot won't be able to apply it. Let me know if there's a better
place for me to put it.

Thanks,
--Jacob

Re: [PATCH] random_normal function

[Tom Lane]

I wrote:
> Michael Paquier  writes:
>> FYI, here is the failure:
>> [21:23:10.814] In file included from pg_prng.c:27:
>> [21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct
>> Node’ declared inside parameter list will not be visible outside of
>> this definition or declaration [-Werror] 
>> [21:23:10.814]46 | struct Node *escontext);

> Hmm ... this looks an awful lot like it is the fault of ccff2d20e
> not of the random_normal patch; that is, we probably need a
> "struct Node" stub declaration in float.h.

[ ... some head-scratching later ... ]

No, we don't per our normal headerscheck rules, which are that
headers such as utils/float.h need to be compilable after including
just postgres.h.  The "struct Node" stub declaration in elog.h will
be visible, making the declaration of float8in_internal kosher.

So the problem in this patch is that it's trying to include
utils/float.h in a src/common file, where we have not included
postgres.h.  Question is, why did you do that?  I see nothing in
pg_prng_double_normal() that looks like it should require that header.
If it did, it'd be questionable whether it's okay to be in src/common.

regards, tom lane

Re: Add index scan progress to pg_stat_progress_vacuum

[Imseih (AWS), Sami]

> cirrus-ci.com/task/4557389261701120

I earlier compiled without building with --enable-cassert,
which is why the compilation errors did not produce on my
buid.

Fixed in v19.

Thanks

--
Sami Imseih
Amazon Web Services: https://aws.amazon.com





v19-0001-Add-2-new-columns-to-pg_stat_progress_vacuum.-Th.patch
Description: v19-0001-Add-2-new-columns-to-pg_stat_progress_vacuum.-Th.patch

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Andrew Dunstan]

On 2023-01-02 Mo 10:57, Tom Lane wrote:
> Corey Huinker  writes:
>> The proposed changes are as follows:
>> CAST(expr AS typename)
>> continues to behave as before.
>> CAST(expr AS typename ERROR ON ERROR)
>> has the identical behavior as the unadorned CAST() above.
>> CAST(expr AS typename NULL ON ERROR)
>> will use error-safe functions to do the cast of expr, and will return
>> NULL if the cast fails.
>> CAST(expr AS typename DEFAULT expr2 ON ERROR)
>> will use error-safe functions to do the cast of expr, and will return
>> expr2 if the cast fails.
> While I approve of trying to get some functionality in this area,
> I'm not sure that extending CAST is a great idea, because I'm afraid
> that the SQL committee will do something that conflicts with it.
> If we know that they are about to standardize exactly this syntax,
> where is that information available?  If we don't know that,
> I'd prefer to invent some kind of function or other instead of
> extending the grammar.


+1


cheers


andrew

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

Re: Stack overflow issue

[Sascha Kuhl]

Great work. Max Stack depth is memory dependent? Processor dependent?

Егор Чиндяскин  schrieb am Mi., 24. Aug. 2022, 11:51:

> Hello, I recently got a server crash (bug #17583 [1]) caused by a stack
> overflow.
>
> Tom Lane and Richard Guo, in a discussion of this bug, suggested that
> there could be more such places.
> Therefore, Alexander Lakhin and I decided to deal with this issue and
> Alexander developed a methodology. We processed src/backend/*/*.c with
> "clang -emit-llvm  ... | opt -analyze -print-calgraph" to find all the
> functions that call themselves directly. I checked each of them for
> features that protect against stack overflows.
> We analyzed 4 catalogs: regex, tsearch, snowball and adt.
> Firstly, we decided to test the regex catalog functions and found 6 of
> them that lack the check_stach_depth() call.
>
> zaptreesubs
> markst
> next
> nfatree
> numst
> repeat
>
> We have tried to exploit the recursion in the function zaptreesubs():
> select regexp_matches('a' || repeat(' a', 11000), '(.)(' || repeat(' \1',
> 11000) || ')?');
>
> ERROR:  invalid regular expression: regular expression is too complex
>
> repeat():
> select regexp_match('abc01234xyz',repeat('a{0,2}',11));
>
> ERROR:  invalid regular expression: regular expression is too complex
>
> numst():
> select regexp_match('abc01234xyz',repeat('(.)\1e',11));
>
> ERROR:  invalid regular expression: regular expression is too complex
>
> markst():
> markst is called in the code after v->tree = parse(...);
> it is necessary that the tree be successfully parsed, but with a nesting
> level of about 100,000 this will not work - stack protection will work
> during parsing and v->ntree = numst(...); is also there.
>
> next():
> we were able to crash the server with the following query:
> (printf "SELECT regexp_match('abc', 'a"; for ((i=1;i<100;i++)); do
> printf "(?#)"; done; printf "b')" ) | psql
>
> Secondly, we have tried to exploit the recursion in the adt catalog
> functions and Alexander was able to crash the server with the following
> query:
>
> regex_selectivity_sub():
> SELECT * FROM pg_proc WHERE proname ~ ('(a' || repeat('|', 20) ||
> 'b)');
>
> And this query:
>
> (n=10;
> printf "SELECT polygon '((0,0),(0,100))' <@ polygon
> '((-20,100),";
> for ((i=1;i<$n;i++)); do printf "(10,$(( 30 +
> $i))),(-10,$((80 + $i))),"; done;
> printf "(20,90),(20,0))';"
> ) | psql
>
> Thirdly, the snowball catalog, Alexander has tried to exploit the
> recursion in the r_stem_suffix_chain_before_ki function and crashed a
> server using this query:
>
> r_stem_suffix_chain_before_ki():
> SELECT ts_lexize('turkish_stem', repeat('lerdeki', 100));
>
> The last one is the tsearch catalog. We have found 4 functions that didn't
> have check_stach_depth() function:
>
> SplitToVariants
> mkANode
> mkSPNode
> LexizeExec
>
> We have tried to exploit the recursion in the SplitToVariants function and
> Alexander crashed a server using this:
>
> SplitToVariants():
> CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell,
> DictFile=ispell_sample,AffFile=ispell_sample);
> SELECT ts_lexize('ispell', repeat('bally', 1));
>
> After trying to exploit the recursion in the LexizeExec function Alexander
> made this conlusion:
>
> LexizeExec has two branches "ld->curDictId == InvalidOid" (usual mode) and
> "ld->curDictId != InvalidOid" (multiword mode) - we start with the first
> one, then make recursive call to switch to the multiword mode, but then we
> return to the usual mode again.
>
> mkANode and mkSPNode deal with the dictionary structs, not with
> user-supplied data, so we believe these functions are not vulnerable.
>
> [1]
> https://www.postgresql.org/message-id/flat/CAMbWs499ytQiH4mLMhRxRWP-iEUz3-DSinpAD-cUCtVo_23Wtg%40mail.gmail.com#03ad703cf4bc8d28ccba69913e1e8106
>

Re: recovery modules

[Nathan Bossart]

Here is a rebased patch set for cfbot.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 7fecc9c9dc8a0ebbfbb1828a8410dac1be1ce7f5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 23 Dec 2022 16:35:25 -0800
Subject: [PATCH v3 1/3] Move the code to restore files via the shell to a
 separate file.

This is preparatory work for allowing more extensibility in this
area.
---
 src/backend/access/transam/Makefile|   1 +
 src/backend/access/transam/meson.build |   1 +
 src/backend/access/transam/shell_restore.c | 194 +
 src/backend/access/transam/xlog.c  |  44 -
 src/backend/access/transam/xlogarchive.c   | 158 +
 src/include/access/xlogarchive.h   |   7 +-
 6 files changed, 240 insertions(+), 165 deletions(-)
 create mode 100644 src/backend/access/transam/shell_restore.c

diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile
index 661c55a9db..099c315d03 100644
--- a/src/backend/access/transam/Makefile
+++ b/src/backend/access/transam/Makefile
@@ -19,6 +19,7 @@ OBJS = \
 	multixact.o \
 	parallel.o \
 	rmgr.o \
+	shell_restore.o \
 	slru.o \
 	subtrans.o \
 	timeline.o \
diff --git a/src/backend/access/transam/meson.build b/src/backend/access/transam/meson.build
index 8920c1bfce..3031c2f6cf 100644
--- a/src/backend/access/transam/meson.build
+++ b/src/backend/access/transam/meson.build
@@ -7,6 +7,7 @@ backend_sources += files(
   'multixact.c',
   'parallel.c',
   'rmgr.c',
+  'shell_restore.c',
   'slru.c',
   'subtrans.c',
   'timeline.c',
diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c
new file mode 100644
index 00..3ddcabd969
--- /dev/null
+++ b/src/backend/access/transam/shell_restore.c
@@ -0,0 +1,194 @@
+/*-
+ *
+ * shell_restore.c
+ *
+ * These recovery functions use a user-specified shell command (e.g., the
+ * restore_command GUC).
+ *
+ * Copyright (c) 2022, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/backend/access/transam/shell_restore.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include 
+
+#include "access/xlogarchive.h"
+#include "access/xlogrecovery.h"
+#include "common/archive.h"
+#include "storage/ipc.h"
+#include "utils/wait_event.h"
+
+static void ExecuteRecoveryCommand(const char *command,
+   const char *commandName, bool failOnSignal,
+   uint32 wait_event_info,
+   const char *lastRestartPointFileName);
+
+bool
+shell_restore(const char *file, const char *path,
+			  const char *lastRestartPointFileName)
+{
+	char	   *cmd;
+	int			rc;
+
+	/* Build the restore command to execute */
+	cmd = BuildRestoreCommand(recoveryRestoreCommand, path, file,
+			  lastRestartPointFileName);
+	if (cmd == NULL)
+		elog(ERROR, "could not build restore command \"%s\"", cmd);
+
+	ereport(DEBUG3,
+			(errmsg_internal("executing restore command \"%s\"", cmd)));
+
+	/*
+	 * Copy xlog from archival storage to XLOGDIR
+	 */
+	fflush(NULL);
+	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+	rc = system(cmd);
+	pgstat_report_wait_end();
+
+	pfree(cmd);
+
+	/*
+	 * Remember, we rollforward UNTIL the restore fails so failure here is
+	 * just part of the process... that makes it difficult to determine
+	 * whether the restore failed because there isn't an archive to restore,
+	 * or because the administrator has specified the restore program
+	 * incorrectly.  We have to assume the former.
+	 *
+	 * However, if the failure was due to any sort of signal, it's best to
+	 * punt and abort recovery.  (If we "return false" here, upper levels will
+	 * assume that recovery is complete and start up the database!) It's
+	 * essential to abort on child SIGINT and SIGQUIT, because per spec
+	 * system() ignores SIGINT and SIGQUIT while waiting; if we see one of
+	 * those it's a good bet we should have gotten it too.
+	 *
+	 * On SIGTERM, assume we have received a fast shutdown request, and exit
+	 * cleanly. It's pure chance whether we receive the SIGTERM first, or the
+	 * child process. If we receive it first, the signal handler will call
+	 * proc_exit, otherwise we do it here. If we or the child process received
+	 * SIGTERM for any other reason than a fast shutdown request, postmaster
+	 * will perform an immediate shutdown when it sees us exiting
+	 * unexpectedly.
+	 *
+	 * We treat hard shell errors such as "command not found" as fatal, too.
+	 */
+	if (wait_result_is_signal(rc, SIGTERM))
+		proc_exit(1);
+
+	ereport(wait_result_is_any_signal(rc, true) ? FATAL : DEBUG2,
+			(errmsg("could not restore file \"%s\" from archive: %s",
+	file, wait_result_to_str(rc;
+
+	return (rc == 0);
+}
+
+void
+shell_archive_cleanup(const char *lastRestartPointFileName)
+{
+	ExecuteRecoveryCommand(archiveCleanupCommand, "archive_cleanup_command"

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Corey Huinker]

On Mon, Jan 2, 2023 at 10:57 AM Tom Lane  wrote:

> Corey Huinker  writes:
> > The proposed changes are as follows:
> > CAST(expr AS typename)
> > continues to behave as before.
> > CAST(expr AS typename ERROR ON ERROR)
> > has the identical behavior as the unadorned CAST() above.
> > CAST(expr AS typename NULL ON ERROR)
> > will use error-safe functions to do the cast of expr, and will return
> > NULL if the cast fails.
> > CAST(expr AS typename DEFAULT expr2 ON ERROR)
> > will use error-safe functions to do the cast of expr, and will return
> > expr2 if the cast fails.
>
> While I approve of trying to get some functionality in this area,
> I'm not sure that extending CAST is a great idea, because I'm afraid
> that the SQL committee will do something that conflicts with it.
> If we know that they are about to standardize exactly this syntax,
> where is that information available?  If we don't know that,
> I'd prefer to invent some kind of function or other instead of
> extending the grammar.
>
> regards, tom lane
>

I'm going off the spec that Vik presented in
https://www.postgresql.org/message-id/f8600a3b-f697-2577-8fea-f40d3e18b...@postgresfriends.org
which is his effort to get it through the SQL committee. I was
alreading thinking about how to get the SQLServer TRY_CAST() function into
postgres, so this seemed like the logical next step.

While the syntax may change, the underlying infrastructure would remain
basically the same: we would need the ability to detect that a typecast had
failed, and replace it with the default value, and handle that at parse
time, or executor time, and handle array casts where the array has the
default but the underlying elements can't.

It would be simple to move the grammar changes to their own patch if that
removes a barrier for people.

Re: wake up logical workers after ALTER SUBSCRIPTION

[Nathan Bossart]

On Tue, Jan 03, 2023 at 11:03:32AM +0530, Amit Kapila wrote:
> On Thu, Dec 15, 2022 at 4:47 AM Nathan Bossart  
> wrote:
>> On Wed, Dec 14, 2022 at 02:02:58PM -0500, Tom Lane wrote:
>> > Maybe we could have workers that are exiting for that reason set a
>> > flag saying "please restart me without delay"?
>>
>> That helps a bit, but there are still delays when starting workers for new
>> subscriptions.  I think we'd need to create a new array in shared memory
>> for subscription OIDs that need their workers started immediately.
> 
> That would be tricky because the list of subscription OIDs can be
> longer than the workers. Can't we set a boolean variable
> (check_immediate or something like that) in LogicalRepCtxStruct and
> use that to traverse the subscriptions? So, when any worker will
> restart because of a parameter change, we can set the variable and
> send a signal to the launcher. The launcher can then check this
> variable to decide whether to start the missing workers for enabled
> subscriptions.

My approach was to add a variable to LogicalRepWorker that indicated
whether a worker needed to be restarted immediately.  While this is a
little weird because the workers array is treated as slots, it worked
nicely for ALTER SUBSCRIPTION.  However, this doesn't help at all for
CREATE SUBSCRIPTION.

IIUC you are suggesting just one variable that would bypass
wal_retrieve_retry_interval for all subscriptions, not just those newly
altered or created.  This definitely seems like it would prevent delays,
but it would also cause wal_retrieve_retry_interval to be incorrectly
bypassed for the other workers in some cases.  Is this acceptable?

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

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Tom Lane]

Corey Huinker  writes:
> On Mon, Jan 2, 2023 at 10:57 AM Tom Lane  wrote:
>> While I approve of trying to get some functionality in this area,
>> I'm not sure that extending CAST is a great idea, because I'm afraid
>> that the SQL committee will do something that conflicts with it.

> I'm going off the spec that Vik presented in
> https://www.postgresql.org/message-id/f8600a3b-f697-2577-8fea-f40d3e18b...@postgresfriends.org
> which is his effort to get it through the SQL committee.

I'm pretty certain that sending something to pgsql-hackers will have
exactly zero impact on the SQL committee.  Is there anything actually
submitted to the committee, and if so what's its status?

regards, tom lane

Re: Error-safe user functions

[Robert Haas]

On Sun, Dec 25, 2022 at 12:13 PM Tom Lane  wrote:
> Here's a proposed patch for converting regprocin and friends
> to soft error reporting.  I'll say at the outset that it's an
> engineering compromise, and it may be worth going further in
> future.  But I doubt it's worth doing more than this for v16,
> because the next steps would be pretty invasive.

I don't know that I feel particularly good about converting some
errors to be reported softly and others not, especially since the
dividing line around which things fall into which category is pretty
much "well, whatever seemed hard we didn't convert". We could consider
hanging it to report everything as a hard error until we can convert
everything, but I'm not sure that's better.

On another note, I can't help noticing that all of these patches seem
to have been committed without any documentation changes. Maybe that's
because there's nothing user-visible that makes any use of these
features yet, but if that's true, then we probably ought to add
something so that the changes are testable. And having done that we
need to explain to users what the behavior actually is: that input
validation errors are trapped but other kinds of failures like out of
memory are not; that most core data types report all input validation
errors softly, and the exceptions; and that for non-core data types
the behavior depends on how the extension is coded. I think it's
really a mistake to suppose that users won't care about or don't need
to know these kinds of details. In my experience, that's just not
true.

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

Re: wake up logical workers after ALTER SUBSCRIPTION

[Nathan Bossart]

On Tue, Jan 03, 2023 at 11:43:59AM +0530, Amit Kapila wrote:
> On Wed, Dec 7, 2022 at 11:42 PM Nathan Bossart  
> wrote:
>> After sleeping on this, I think we can do better.  IIUC we can simply check
>> for AllTablesyncsReady() at the end of process_syncing_tables_for_apply()
>> and wake up the logical replication workers (which should just consiѕt of
>> setting the current process's latch) if we are ready for two_phase mode.
> 
> How just waking up will help with two_phase mode? For that, we need to
> restart the apply worker as we are doing at the beginning of
> process_syncing_tables_for_apply().

Right.  IIRC waking up causes the apply worker to immediately call
process_syncing_tables_for_apply() again, which will then proc_exit(0) as
appropriate.  It might be possible to move the restart logic to the end of
process_syncing_tables_for_apply() to avoid this extra wakeup.  WDYT?

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

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Vik Fearing]

On 1/3/23 19:14, Tom Lane wrote:

Corey Huinker  writes:

On Mon, Jan 2, 2023 at 10:57 AM Tom Lane  wrote:

While I approve of trying to get some functionality in this area,
I'm not sure that extending CAST is a great idea, because I'm afraid
that the SQL committee will do something that conflicts with it.



I'm going off the spec that Vik presented in
https://www.postgresql.org/message-id/f8600a3b-f697-2577-8fea-f40d3e18b...@postgresfriends.org
which is his effort to get it through the SQL committee.


I'm pretty certain that sending something to pgsql-hackers will have
exactly zero impact on the SQL committee.  Is there anything actually
submitted to the committee, and if so what's its status?


I have not posted my paper to the committee yet, but I plan to do so 
before the working group's meeting early February.  Just like with 
posting patches here, I cannot guarantee that it will get accepted but I 
will be arguing for it.


I don't think we should add that syntax until I do get it through the 
committee, just in case they change something.

--
Vik Fearing

Re: Simplify standby state machine a bit in WaitForWALToBecomeAvailable()

[Nathan Bossart]

On Tue, Jan 03, 2023 at 02:53:10PM +0530, Bharath Rupireddy wrote:
> In summary:
> the flow when the standby is in crash recovery is pg_wal -> [archive
> -> pg_wal -> stream] -> [archive -> pg_wal -> stream] -> [] -> [] ...
> the flow when the standby is in archive recovery is [archive -> pg_wal
> -> stream] -> [archive -> pg_wal -> stream] -> [] -> [] ...

This is my understanding as well.
 
> The proposed patch makes the inherent state change to pg_wal after
> failure to read from archive in XLogFileReadAnyTLI() to explicit by
> setting currentSource to XLOG_FROM_PG_WAL in the state machine. I
> think it doesn't alter the existing state machine or add any new extra
> lookups in pg_wal.

I'm assuming this change would simplify your other patch that modifieѕ
WaitForWALToBecomeAvailable() [0].  Is that correct?

[0] https://commitfest.postgresql.org/41/3663/

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

Re: enable_timeout_every() and fin_time

[Robert Haas]

On Sun, Jan 1, 2023 at 7:36 PM Andres Freund  wrote:
> What is the use case for an absolute start time plus a relative
> interval?

The code snippet that you indicate has the important side effect of
changing the global variable startup_progress_phase_start_time, which
is used by has_startup_progress_timeout_expired. Without the fin_time
argument, the timeout machinery would have to call
GetCurrentTimestamp() separately, and the caller wouldn't know what
answer it got. The result would be that the progress reports would
indicate an elapsed time relative to one timestamp, but the time at
which those progress reports were printed would be relative to a
slightly different timestamp.

Maybe nobody would notice such a minor discrepancy, but I wanted to avoid it.

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

Re: [PATCH] Expand character set for ltree labels

[Garen Torikian]

Sure. Rebased onto HEAD.

On Tue, Jan 3, 2023 at 7:27 AM vignesh C  wrote:

> On Thu, 6 Oct 2022 at 03:35, Garen Torikian  wrote:
> >
> > 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
>
> The patch does not apply on top of HEAD as in [1], please post a rebased
> patch:
> === Applying patches on top of PostgreSQL commit ID
> e351f85418313e97c203c73181757a007dfda6d0 ===
> === applying patch ./0002-Expand-character-set-for-ltree-labels.patch
> patching file contrib/ltree/expected/ltree.out
> patching file contrib/ltree/ltree.h
> Hunk #2 FAILED at 126.
> 1 out of 2 hunks FAILED -- saving rejects to file contrib/ltree/ltree.h.rej
>
> [1] - http://cfbot.cputube.org/patch_41_3929.log
>
> Regards,
> Vignesh
>


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

Re: recovery modules

[Nathan Bossart]

On Tue, Jan 03, 2023 at 09:59:17AM -0800, Nathan Bossart wrote:
> Here is a rebased patch set for cfbot.

I noticed that cfbot's Windows tests are failing because the backslashes in
the archive directory path are causing escaping problems.  Here is an
attempt to fix that by converting all backslashes to forward slashes, which
is what other tests (e.g., 025_stuck_on_old_timeline.pl) do.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 7fecc9c9dc8a0ebbfbb1828a8410dac1be1ce7f5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 23 Dec 2022 16:35:25 -0800
Subject: [PATCH v4 1/3] Move the code to restore files via the shell to a
 separate file.

This is preparatory work for allowing more extensibility in this
area.
---
 src/backend/access/transam/Makefile|   1 +
 src/backend/access/transam/meson.build |   1 +
 src/backend/access/transam/shell_restore.c | 194 +
 src/backend/access/transam/xlog.c  |  44 -
 src/backend/access/transam/xlogarchive.c   | 158 +
 src/include/access/xlogarchive.h   |   7 +-
 6 files changed, 240 insertions(+), 165 deletions(-)
 create mode 100644 src/backend/access/transam/shell_restore.c

diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile
index 661c55a9db..099c315d03 100644
--- a/src/backend/access/transam/Makefile
+++ b/src/backend/access/transam/Makefile
@@ -19,6 +19,7 @@ OBJS = \
 	multixact.o \
 	parallel.o \
 	rmgr.o \
+	shell_restore.o \
 	slru.o \
 	subtrans.o \
 	timeline.o \
diff --git a/src/backend/access/transam/meson.build b/src/backend/access/transam/meson.build
index 8920c1bfce..3031c2f6cf 100644
--- a/src/backend/access/transam/meson.build
+++ b/src/backend/access/transam/meson.build
@@ -7,6 +7,7 @@ backend_sources += files(
   'multixact.c',
   'parallel.c',
   'rmgr.c',
+  'shell_restore.c',
   'slru.c',
   'subtrans.c',
   'timeline.c',
diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c
new file mode 100644
index 00..3ddcabd969
--- /dev/null
+++ b/src/backend/access/transam/shell_restore.c
@@ -0,0 +1,194 @@
+/*-
+ *
+ * shell_restore.c
+ *
+ * These recovery functions use a user-specified shell command (e.g., the
+ * restore_command GUC).
+ *
+ * Copyright (c) 2022, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/backend/access/transam/shell_restore.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include 
+
+#include "access/xlogarchive.h"
+#include "access/xlogrecovery.h"
+#include "common/archive.h"
+#include "storage/ipc.h"
+#include "utils/wait_event.h"
+
+static void ExecuteRecoveryCommand(const char *command,
+   const char *commandName, bool failOnSignal,
+   uint32 wait_event_info,
+   const char *lastRestartPointFileName);
+
+bool
+shell_restore(const char *file, const char *path,
+			  const char *lastRestartPointFileName)
+{
+	char	   *cmd;
+	int			rc;
+
+	/* Build the restore command to execute */
+	cmd = BuildRestoreCommand(recoveryRestoreCommand, path, file,
+			  lastRestartPointFileName);
+	if (cmd == NULL)
+		elog(ERROR, "could not build restore command \"%s\"", cmd);
+
+	ereport(DEBUG3,
+			(errmsg_internal("executing restore command \"%s\"", cmd)));
+
+	/*
+	 * Copy xlog from archival storage to XLOGDIR
+	 */
+	fflush(NULL);
+	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+	rc = system(cmd);
+	pgstat_report_wait_end();
+
+	pfree(cmd);
+
+	/*
+	 * Remember, we rollforward UNTIL the restore fails so failure here is
+	 * just part of the process... that makes it difficult to determine
+	 * whether the restore failed because there isn't an archive to restore,
+	 * or because the administrator has specified the restore program
+	 * incorrectly.  We have to assume the former.
+	 *
+	 * However, if the failure was due to any sort of signal, it's best to
+	 * punt and abort recovery.  (If we "return false" here, upper levels will
+	 * assume that recovery is complete and start up the database!) It's
+	 * essential to abort on child SIGINT and SIGQUIT, because per spec
+	 * system() ignores SIGINT and SIGQUIT while waiting; if we see one of
+	 * those it's a good bet we should have gotten it too.
+	 *
+	 * On SIGTERM, assume we have received a fast shutdown request, and exit
+	 * cleanly. It's pure chance whether we receive the SIGTERM first, or the
+	 * child process. If we receive it first, the signal handler will call
+	 * proc_exit, otherwise we do it here. If we or the child process received
+	 * SIGTERM for any other reason than a fast shutdown request, postmaster
+	 * will perform an immediate shutdown when it sees us exiting
+	 * unexpectedly.
+	 *
+	 * We treat hard shell errors such as "command not found" as fatal, too.
+	 */
+	if (wait_result_is_signal(rc, SIGTERM))
+		p

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

[Tom Lane]

Amit Langote  writes:
> Updated to replace a list_nth() with list_nth_node() and rewrote the
> commit message.

So I was working through this with intent to commit, when I realized
that the existing code it's revising is flat out broken.  You can't
simply translate a parent rel's set of dependent generated columns
to obtain the correct set for a child.  Maybe that's sufficient for
partitioned tables, but it fails miserably for general inheritance:

regression=# create table pp(f1 int);
CREATE TABLE
regression=# create table cc(f2 int generated always as (f1+1) stored) 
inherits(pp);
CREATE TABLE
regression=# insert into cc values(42);
INSERT 0 1
regression=# table cc;
 f1 | f2 
+
 42 | 43
(1 row)

regression=# update pp set f1 = f1*10;
UPDATE 1
regression=# table cc;
 f1  | f2 
-+
 420 | 43
(1 row)

So we have a long-standing existing bug to fix here.

I think what we have to do basically is repeat what fill_extraUpdatedCols
does independently for each target table.  That's not really horrible:
given the premise that we're moving this calculation into the planner,
we can have expand_single_inheritance_child run the code while we have
each target table open.  It'll require some rethinking though, and we
will need to have the set of update target columns already available
at that point.  This suggests that we want to put the updated_cols and
extraUpdatedCols fields into RelOptInfo not PlannerInfo.

regards, tom lane

Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions

[Tom Lane]

Vik Fearing  writes:
> I have not posted my paper to the committee yet, but I plan to do so 
> before the working group's meeting early February.  Just like with 
> posting patches here, I cannot guarantee that it will get accepted but I 
> will be arguing for it.

> I don't think we should add that syntax until I do get it through the 
> committee, just in case they change something.

Agreed.  So this is something we won't be able to put into v16;
it'll have to wait till there's something solid from the committee.

regards, tom lane

Re: allowing for control over SET ROLE

[Robert Haas]

On Sat, Dec 31, 2022 at 1:16 AM Noah Misch  wrote:
> On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
> > On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
> > > But I think the bigger reason is that, in my opinion, this proposal is
> > > more generally useful, because it takes no position on why you wish to
> > > disallow SET ROLE.  You can just disallow it in some cases and allow it in
> > > others, and that's fine.
>
> In this commit 3d14e17, the documentation takes the above "no position".  The
> implementation does not, in that WITH SET FALSE has undocumented ability to
> block ALTER ... OWNER TO, not just SET ROLE.  Leaving that undocumented feels
> weird to me, but documenting it would take the position that WITH SET FALSE is
> relevant to the security objective of preventing object creation like the
> example in the original post of this thread.  How do you weigh those
> documentation trade-offs?

In general, I favor trying to make the documentation clearer and more
complete. Intentionally leaving things undocumented doesn't seem like
the right course of action to me. That said, the pre-existing
documentation in this area is so incomplete that it's sometimes hard
to figure out where to add new information - and it made no mention of
the privileges required for ALTER .. OWNER TO. I didn't immediately
know where to add that, so did nothing. Maybe I should have tried
harder, though.

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

Re: [PATCH] pg_dump: lock tables in batches

[Gilles Darold]

Le 08/12/2022 à 01:03, Tom Lane a écrit :

Andres Freund  writes:

On 2022-12-07 17:53:05 -0500, Tom Lane wrote:

Is "-s" mode actually a relevant criterion here?  With per-table COPY
commands added into the mix you could not possibly get better than 2x
improvement, and likely a good deal less.

Well, -s isn't something used all that rarely, so it'd not be insane to
optimize it in isolation. But more importantly, I think the potential win
without -s is far bigger than 2x, because the COPYs can be done in parallel,
whereas the locking happens in the non-parallel stage.

True, and there's the reduce-the-lock-window issue that Jacob mentioned.


With just a 5ms delay, very well within normal network latency range, I get:
[ a nice win ]

OK.  I'm struggling to figure out why I rejected this idea last year.
I know that I thought about it and I'm fairly certain I actually
tested it.  Maybe I only tried it with near-zero-latency local
loopback; but I doubt that, because the potential for network
latency was certainly a factor in that whole discussion.

One idea is that I might've tried it before getting rid of all the
other per-object queries, at which point it wouldn't have stood out
quite so much.  But I'm just guessing.  I have a nagging feeling
there was something else.

Oh well, I guess we can always revert it if we discover a problem later.

regards, tom lane


Hi,


I have done a review of this patch, it applies well on current master 
and compiles without problem.


make check/installcheck and world run without failure, pg_dump tests 
with pgtap enabled work fine too.


I have also given a try to the bench mentioned in the previous posts and 
I have the same performances gain with the -s option.



As it seems to have a consensus to apply this patch I will change the 
commitfest status to ready for committers.



Regards,

--
Gilles Darold

Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier

[Andres Freund]

Hi,

On 2022-12-30 14:14:55 +1300, Thomas Munro wrote:
> Oh, I was imagining something slightly different.  Not something under
> src/include/libpq, but conceptually a separate header-only library
> that is above both the backend and libpq.  Maybe something like
> src/include/febe_util/libpq_connect_interruptible.h.  In other words,
> I thought your idea b was a header-only version of your idea a.  I
> think that might be a bit nicer than putting it under libpq?
> Superficial difference, perhaps...

It doesn't seem entirely right to introduce a top-level "module" for
libpq-in-extensions to me - we don't do that for other APIs used for
extensions. But a header only library also doesn't quite seem right. So ...

Looking at turning my patch upthread into something slightly less prototype-y,
I noticed that libpqwalreceiver doesn't do AcquireExternalFD(), added to other
backend uses of libpq in 3d475515a15. It's unlikely to matter for
walreceiver.c itelf, but it seems problematic for the logical replication
cases?

It's annoying to introduce PG_TRY/CATCH blocks, just to deal with the
potential for errors inside WaitLatchOrSocket(), which should never happen. I
wonder if we should consider making latch.c error out fatally, instead of
elog(ERROR). If latches are broken, things are bad.


The PG_CATCH() logic in postgres_fdw's GetConnection() looks quite suspicious
to me. It looks like 32a9c0bdf493 took entirely the wrong path. Instead of
optionally not throwing or directly re-establishing connections in
begin_remote_xact(), the PG_CATCH() hammer was applied.



The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
dblink, postgres_fdw.

As I made libpq-be-fe-helpers.h handle reserving external fds,
libpqwalreceiver now does so. I briefly looked through its users without
seeing cases of leaking in case of errors - which would already have been bad,
since we'd already have leaked a libpq connection/socket.


Given the lack of field complaints and the size of the required changes, I
don't think we should backpatch this, even though it's pretty clearly buggy
as-is.


Some time back Thomas had a patch to introduce a wrapper around
libpq-in-extensions that fixed issues cause by some events being
edge-triggered on windows. It's possible that combining these two efforts
would yield something better. I resisted the urge to create a wrapper around
each connection in this patch, as it'd have ended up being a whole lot more
invasive. But... Thomas, do you have a reference to that code handy?

Greetings,

Andres Freund
>From 34e5ea311322b3bc3bb0f8c925f9ade1a59c6f09 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 3 Jan 2023 11:31:04 -0800
Subject: [PATCH v2] wip: don't block inside PQconnectdb et al

---
 src/include/libpq/libpq-be-fe-helpers.h   | 233 ++
 .../libpqwalreceiver/libpqwalreceiver.c   |  53 +---
 contrib/dblink/dblink.c   |  80 +-
 contrib/postgres_fdw/connection.c |  42 +---
 4 files changed, 256 insertions(+), 152 deletions(-)
 create mode 100644 src/include/libpq/libpq-be-fe-helpers.h

diff --git a/src/include/libpq/libpq-be-fe-helpers.h b/src/include/libpq/libpq-be-fe-helpers.h
new file mode 100644
index 000..4f3d3b821f0
--- /dev/null
+++ b/src/include/libpq/libpq-be-fe-helpers.h
@@ -0,0 +1,233 @@
+/*-
+ *
+ * libpq-be-fe-helpers.h
+ *	  Helper functions for using libpq in extensions
+ *
+ * Code built directly into the backend is not allowed to link to libpq
+ * directly. Extension code is allowed to use libpq however. However, libpq
+ * used in extensions has to be careful to block inside libpq, otherwise
+ * interrupts will not be processed, leading to issues like unresolvable
+ * deadlocks. Backend code also needs to take care to acquire/release an
+ * external fd for the connection, otherwise fd.c's accounting of fd's is
+ * broken.
+ *
+ * This file provides helper functions to make it easier to comply with these
+ * rules. It is a header only library as it needs to be linked into each
+ * extension using libpq, and it seems too small to be worth adding a
+ * dedicated static library for.
+ *
+ * TODO: For historical reasons the connections established here are not put
+ * into non-blocking mode. That can lead to blocking even when only the async
+ * libpq functions are used. This should be fixed.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/libpq/libpq-be-fe-helpers.h
+ *
+ *-
+ */
+#ifndef LIBPQ_BE_FE_HELPERS_H
+#define LIBPQ_BE_FE_HELPERS_H
+
+/*
+ * Despite the name, BUILDING_DLL is set only when building code directly part
+ * of the backend. Which also is where libpq isn't allowed to be
+ * used. Obviously this doesn't protect agains

Re: fixing CREATEROLE

[Robert Haas]

On Fri, Dec 23, 2022 at 4:55 PM Robert Haas  wrote:
> On Thu, Dec 22, 2022 at 9:14 AM Alvaro Herrera  
> wrote:
> > The contents looks good to me other than that problem, and I agree to
> > backpatch it.
>
> Cool. Thanks for the review.
>
> > Why did you choose to use two dots for ellipses in some command
> > s rather than three?  I know I've made that choice too on
> > occassion, but there aren't many such cases and maybe we should put a
> > stop to it (or a period) before it spreads too much.
>
> Honestly, I wasn't aware that we had some other convention for it.

Committed and back-patched 0001 with fixes for the issues that you pointed out.

Here's a trivial rebase of the rest of the patch set.

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


v3-0001-Refactor-permissions-checking-for-role-grants.patch
Description: Binary data


v3-0004-Add-new-GUC-createrole_self_grant.patch
Description: Binary data


v3-0002-Pass-down-current-user-ID-to-AddRoleMems-and-DelR.patch
Description: Binary data


v3-0003-Restrict-the-privileges-of-CREATEROLE-users.patch
Description: Binary data

Re: enable_timeout_every() and fin_time

[Andres Freund]

Hi,

On 2023-01-03 13:33:34 -0500, Robert Haas wrote:
> On Sun, Jan 1, 2023 at 7:36 PM Andres Freund  wrote:
> > What is the use case for an absolute start time plus a relative
> > interval?
> 
> The code snippet that you indicate has the important side effect of
> changing the global variable startup_progress_phase_start_time, which
> is used by has_startup_progress_timeout_expired. Without the fin_time
> argument, the timeout machinery would have to call
> GetCurrentTimestamp() separately, and the caller wouldn't know what
> answer it got. The result would be that the progress reports would
> indicate an elapsed time relative to one timestamp, but the time at
> which those progress reports were printed would be relative to a
> slightly different timestamp.

> Maybe nobody would notice such a minor discrepancy, but I wanted to avoid it.

Doesn't that discrepancy already exist as the code stands, because
startup_progress_phase_start_time is also set in
has_startup_progress_timeout_expired()? I realize that was an example, but the
issue seems broader: After the first "firing", the next timeout will be
computed relative to an absolute time gathered in timestamp.c.

Greetings,

Andres Freund

Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier

[Robert Haas]

On Sun, Sep 25, 2022 at 7:22 PM Andres Freund  wrote:
> Maybe I am missing something, but I don't think it's OK for
> connect_pg_server() to connect in a blocking manner, without accepting
> interrupts?

I remember noticing various problems in this area years ago. I'm not
sure whether I noticed this particular one, but the commit message for
ae9bfc5d65123aaa0d1cca9988037489760bdeae mentions a few others that I
did notice.

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

Why are we using blocking libpq in the backend?

[Andres Freund]

Hi,

As noted in [1], and also commented on by other previously, we use libpq in
blocking mode in libpqwalreceiver, postgres_fdw, etc.  This is done knowingly,
see e.g. comments like:
/*
 * Submit the query.  Since we don't use non-blocking mode, this could
 * theoretically block.  In practice, since we don't send very long 
query
 * strings, the risk seems negligible.
 */

but I don't understand why we do it. It seems like it'd be a fairly small
amount of additional code to just do it right, given that we do so for calls
to PQgetResult() etc?

Greetings,

Andres Freund

[1] https://postgr.es/m/20230103200520.di5hjebqvi72coql%40awork3.anarazel.de

Re: enable_timeout_every() and fin_time

[Robert Haas]

On Tue, Jan 3, 2023 at 3:14 PM Andres Freund  wrote:
> Doesn't that discrepancy already exist as the code stands, because
> startup_progress_phase_start_time is also set in
> has_startup_progress_timeout_expired()?

I don't think it is, actually.

> I realize that was an example, but the
> issue seems broader: After the first "firing", the next timeout will be
> computed relative to an absolute time gathered in timestamp.c.

We're computing the time since the start of the current phase, not the
time since the last timeout. So I don't see how this is relevant.

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

Re: doc: add missing "id" attributes to extension packaging page

[Brar Piening]

On 02.01.2023 at 21:53, Karl O. Pinc wrote:

If the author will look over my version of the patch I believe it can
be approved and sent on to the committers.


LGTM.

Thanks!

Brar

Re: heapgettup refactoring

[Melanie Plageman]

On Mon, Jan 2, 2023 at 5:23 AM Peter Eisentraut
 wrote:
>
> On 30.11.22 23:34, Melanie Plageman wrote:
> > I have attached a patchset with only the code changes contained in the
> > previous patch 0003. I have broken the refactoring down into many
> > smaller pieces for ease of review.
>
> To keep this moving along a bit, I have committed your 0002, which I
> think is a nice little improvement on its own.

Thanks!
I've attached a rebased patchset - v4.

I also changed heapgettup_no_movement() to noinline (instead of inline).
David Rowley pointed out that this might make more sense given how
comparatively rare no movement scans are.

- Melanie


v4-0004-Add-heapgettup-helpers.patch
Description: Binary data


v4-0002-Add-heapgettup_initial_page-helper.patch
Description: Binary data


v4-0005-Add-heapgettup_advance_page.patch
Description: Binary data


v4-0006-Refactor-heapgettup-and-heapgettup_pagemode.patch
Description: Binary data


v4-0003-Streamline-heapgettup-for-refactor.patch
Description: Binary data


v4-0001-Add-no-movement-scan-helper.patch
Description: Binary data

Re: RADIUS tests and improvements

[Andreas Karlsson]

On 1/3/23 04:11, Thomas Munro wrote:

Here's a draft patch to tackle a couple of TODOs in the RADIUS code in auth.c.


Nice to see someone working on this! I know of one company which could 
have used the configurable timeout for radius because the 3 second 
timeout is too short for 2FA. I think they ended up using PAM or some 
other solution in the end, but I am not 100% sure.



[...] While adding
the GUC I couldn't help wondering why RADIUS even needs a timeout
separate from authentication_timeout; another way to go here would be
to remove it completely, but that'd be a policy change (removing the 3
second timeout we always had).  Thoughts?


It was some time since I last looked at the code but my impression was 
that the reason for having a separate timeout is that you can try the 
next server after the first one timed out (multiple radius servers are 
allowed). But I wonder if that really is a useful feature or if someone 
just was too clever or it just was an accidental feature.


Andreas

Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

[Jacob Champion]

On Thu, Dec 8, 2022 at 3:10 PM Jacob Champion  wrote:
> For now, it's worked around in v4. This should finally get the cfbot
> fully green.

Cirrus's switch to M1 Macs changed the Homebrew installation path, so
v5 adjusts the workaround accordingly.

--Jacob
1:  b01812f604 ! 1:  7618037c86 libpq: add sslrootcert=system to use default CAs
@@ .cirrus.yml: task:
 +# the etc/ prefix, so we hardcode the full path here. openssl@3 is 
pinned
 +# above to try to minimize the chances of this changing beneath us, 
but it's
 +# brittle...
-+mkdir -p "/usr/local/etc/openssl@3/certs"
++mkdir -p "/opt/homebrew/etc/openssl@3/certs"
upload_caches: homebrew
  
ccache_cache:
2:  432453942a = 2:  c392e1796e libpq: force sslmode=verify-full for system CAs
From c392e1796e98458d7051bd57a83d221256c84490 Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Mon, 24 Oct 2022 15:24:11 -0700
Subject: [PATCH v5 2/2] libpq: force sslmode=verify-full for system CAs

Weaker verification modes don't make much sense for public CAs.
---
 doc/src/sgml/libpq.sgml   | 15 
 doc/src/sgml/runtime.sgml |  6 ++--
 src/interfaces/libpq/fe-connect.c | 58 +++
 src/interfaces/libpq/t/001_uri.pl | 30 ++--
 src/test/ssl/t/001_ssltests.pl| 12 +++
 5 files changed, 109 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index e3b8a470f8..18c85917d4 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1733,15 +1733,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 locations may be further modified by the SSL_CERT_DIR
 and SSL_CERT_FILE environment variables.

-   
+   
 
- When using sslrootcert=system, it is critical to
- also use the strongest certificate verification method,
- sslmode=verify-full. In most cases it is trivial for
- anyone to obtain a certificate trusted by the system for a hostname
- they control, rendering the verify-ca mode useless.
+ When using sslrootcert=system, the default
+ sslmode is changed to verify-full,
+ and any weaker setting will result in an error. In most cases it is
+ trivial for anyone to obtain a certificate trusted by the system for a
+ hostname they control, rendering verify-ca and all
+ weaker modes useless.
 
-   
+   
   
  
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 0ab93ff398..8db47c3252 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2008,9 +2008,9 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
verify-full and have the appropriate root certificate
file installed (). Alternatively the
system CA pool can be used using sslrootcert=system; in
-   this case, sslmode=verify-full must be used for safety,
-   since it is generally trivial to obtain certificates which are signed by a
-   public CA.
+   this case, sslmode=verify-full is forced for safety, since
+   it is generally trivial to obtain certificates which are signed by a public
+   CA.
   
 
   
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50b5df3490..b5df5d0c46 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1259,6 +1259,23 @@ connectOptions2(PGconn *conn)
 			goto oom_error;
 	}
 
+#ifndef USE_SSL
+	/*
+	 * sslrootcert=system is not supported. Since setting this changes the
+	 * default sslmode, check this _before_ we validate sslmode, to avoid
+	 * confusing the user with errors for an option they may not have set.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0)
+	{
+		conn->status = CONNECTION_BAD;
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("sslrootcert value \"%s\" invalid when SSL support is not compiled in\n"),
+		  conn->sslrootcert);
+		return false;
+	}
+#endif
+
 	/*
 	 * validate sslmode option
 	 */
@@ -1305,6 +1322,22 @@ connectOptions2(PGconn *conn)
 			goto oom_error;
 	}
 
+#ifdef USE_SSL
+	/*
+	 * If sslrootcert=system, make sure our chosen sslmode is compatible.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0
+		&& strcmp(conn->sslmode, "verify-full") != 0)
+	{
+		conn->status = CONNECTION_BAD;
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("weak sslmode \"%s\" may not be used with sslrootcert=system\n"),
+		  conn->sslmode);
+		return false;
+	}
+#endif
+
 	/*
 	 * Validate TLS protocol versions for ssl_min_protocol_version and
 	 * ssl_max_protocol_version.
@@ -5806,6 +5839,7 @@ static bool
 conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 {
 	PQconninfoOption *option;
+	PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL;
 	char	   *tmp;
 
 	/*
@@ -5822,6 +5856,9 @@ conninfo_add_defaults(PQconn

Re: RADIUS tests and improvements

[Andreas Karlsson]

On 1/3/23 22:03, Andreas Karlsson wrote:

On 1/3/23 04:11, Thomas Munro wrote:
Here's a draft patch to tackle a couple of TODOs in the RADIUS code in 
auth.c.


Nice to see someone working on this!.


Another thing: shouldn't we set some wait event to indicate that we are 
waiting the RADIUS server or is that pointless during authentication 
since there are no queries running anyway?


Andreas

Re: RADIUS tests and improvements

[Thomas Munro]

On Wed, Jan 4, 2023 at 10:07 AM Andreas Karlsson  wrote:
> Another thing: shouldn't we set some wait event to indicate that we are
> waiting the RADIUS server or is that pointless during authentication
> since there are no queries running anyway?

I initially added a wait_event value, but I couldn't see it
anywhere... there is no entry in pg_stat_activity for a backend that
is in that phase of authentication, so I just set it to zero.

Re: RADIUS tests and improvements

[Thomas Munro]

On Wed, Jan 4, 2023 at 10:03 AM Andreas Karlsson  wrote:
> On 1/3/23 04:11, Thomas Munro wrote:
> > [...] While adding
> > the GUC I couldn't help wondering why RADIUS even needs a timeout
> > separate from authentication_timeout; another way to go here would be
> > to remove it completely, but that'd be a policy change (removing the 3
> > second timeout we always had).  Thoughts?
>
> It was some time since I last looked at the code but my impression was
> that the reason for having a separate timeout is that you can try the
> next server after the first one timed out (multiple radius servers are
> allowed). But I wonder if that really is a useful feature or if someone
> just was too clever or it just was an accidental feature.

Ah!  Thanks, now that makes sense.

Re: RADIUS tests and improvements

[Andreas Karlsson]

On 1/3/23 22:16, Thomas Munro wrote:

On Wed, Jan 4, 2023 at 10:07 AM Andreas Karlsson  wrote:

Another thing: shouldn't we set some wait event to indicate that we are
waiting the RADIUS server or is that pointless during authentication
since there are no queries running anyway?


I initially added a wait_event value, but I couldn't see it
anywhere... there is no entry in pg_stat_activity for a backend that
is in that phase of authentication, so I just set it to zero.


Thanks for the explanation, that makes a lot of sense!

Andreas

Re: typos

[Justin Pryzby]

On Tue, Jan 03, 2023 at 04:28:29PM +0900, Michael Paquier wrote:
> On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote:
> 
>  # Use larger ccache cache, as this task compiles with multiple compilers 
> /
>  # flag combinations
> -CCACHE_MAXSIZE: "1GB"
> +CCACHE_MAXSIZE: "1G"
> 
> In 0006, I am not sure how much this matters.  Perhaps somebody more
> fluent with Cirrus, though, has a different opinion..

It's got almost nothing to do with cirrus.  It's an environment
variable, and we're using a suffix other than what's
supported/documented by ccache, which only happens to work.

> 0014 and 0013 do not reduce the translation workload, as the messages
> include some stuff specific to the GUC names accessed to, or some
> specific details about the code paths triggered.

It seems to matter because otherwise the translators sometimes re-type
the view name, which (not surprisingly) can get messed up, which is how
I mentioned having noticed this.

On Tue, Jan 03, 2023 at 05:41:58PM +0900, Michael Paquier wrote:
> On Tue, Jan 03, 2023 at 01:03:01PM +0530, Amit Kapila wrote:
> > One minor comment:
> > -spoken in Belgium (BE), with a UTF-8
> > character set
> > +spoken in Belgium (BE), with a UTF-8
> > character set
> > 
> > Shouldn't this be UTF8 as we are using in
> > func.sgml?
> 
> Yeah, I was wondering as well why this change is not worse, which is
> why I left it out of 33ab0a2.  There is an acronym for UTF in
> acronym.sgml, which makes sense to me, but that's the only place where 
> this is used.  To add more on top of that, the docs basically need
> only UTF8, and we have three references to UTF-16, none of them using
> the  markup.

I changed it for consistency, as it's the only thing that says <>UTF-8<>
anywhere, and charset.sgml already says <>UTF<>-8 elsewhere.

Alternately, I suggest to change charset to say <>UTF8<> in both places.

-- 
Justin

Re: allowing for control over SET ROLE

[Noah Misch]

On Tue, Jan 03, 2023 at 02:43:10PM -0500, Robert Haas wrote:
> On Sat, Dec 31, 2022 at 1:16 AM Noah Misch  wrote:
> > On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
> > > On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
> > > > But I think the bigger reason is that, in my opinion, this proposal is
> > > > more generally useful, because it takes no position on why you wish to
> > > > disallow SET ROLE.  You can just disallow it in some cases and allow it 
> > > > in
> > > > others, and that's fine.
> >
> > In this commit 3d14e17, the documentation takes the above "no position".  
> > The
> > implementation does not, in that WITH SET FALSE has undocumented ability to
> > block ALTER ... OWNER TO, not just SET ROLE.  Leaving that undocumented 
> > feels
> > weird to me, but documenting it would take the position that WITH SET FALSE 
> > is
> > relevant to the security objective of preventing object creation like the
> > example in the original post of this thread.  How do you weigh those
> > documentation trade-offs?
> 
> In general, I favor trying to make the documentation clearer and more
> complete. Intentionally leaving things undocumented doesn't seem like
> the right course of action to me.

For what it's worth, I like to leave many things undocumented, but not this.

> That said, the pre-existing
> documentation in this area is so incomplete that it's sometimes hard
> to figure out where to add new information - and it made no mention of
> the privileges required for ALTER .. OWNER TO. I didn't immediately
> know where to add that, so did nothing.

I'd start with locations where the patch already added documentation.  In the
absence of documentation otherwise, a reasonable person could think WITH SET
controls just SET ROLE.  The documentation of WITH SET is a good place to list
what else you opted for it to control.  If the documentation can explain the
set of principles that would be used to decide whether WITH SET should govern
another thing in the future, that would provide extra value.

Re: doc: add missing "id" attributes to extension packaging page

[Karl O. Pinc]

On Tue, 3 Jan 2023 21:35:09 +0100
Brar Piening  wrote:

> On 02.01.2023 at 21:53, Karl O. Pinc wrote:
> > If the author will look over my version of the patch I believe it
> > can be approved and sent on to the committers.  
> 
> LGTM.

Approved for committer!

Regards,

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

Re: [PATCH] pg_dump: lock tables in batches

[Tom Lane]

Gilles Darold  writes:
> As it seems to have a consensus to apply this patch I will change the 
> commitfest status to ready for committers.

Yeah.  I still have a nagging worry about why I didn't do this already,
but without evidence I can't fairly block the patch.  Hence, pushed.

I did cut the LOCK TABLE query length limit from 1MB to 100K, because
I doubt there is any measurable performance difference, and I'm a
little worried about overstressing smaller servers.

regards, tom lane

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

[Nathan Bossart]

On Sun, Dec 18, 2022 at 03:30:18PM -0800, Nathan Bossart wrote:
> Here is a new version of the patch.  Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup.  Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.

I'd like to get this fixed, but I have yet to hear thoughts on the
suggested approach.  I'll proceed with adjusting the tests and
documentation shortly unless someone objects.

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

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

[Tom Lane]

I wrote:
> I think what we have to do basically is repeat what fill_extraUpdatedCols
> does independently for each target table.  That's not really horrible:
> given the premise that we're moving this calculation into the planner,
> we can have expand_single_inheritance_child run the code while we have
> each target table open.  It'll require some rethinking though, and we
> will need to have the set of update target columns already available
> at that point.  This suggests that we want to put the updated_cols and
> extraUpdatedCols fields into RelOptInfo not PlannerInfo.

After further thought: maybe we should get radical and postpone this
work all the way to executor startup.  The downside of that is having
to do it over again on each execution of a prepared plan.  But the
upside is that when the UPDATE targets a many-partitioned table,
we would have a chance at not doing the work at all for partitions
that get pruned at runtime.  I'm not sure if that win would emerge
immediately or if we still have executor work to do to manage pruning
of the target table.  I'm also not sure that this'd be a net win
overall.  But it seems worth considering.

regards, tom lane

Re: Common function for percent placeholder replacement

[Nathan Bossart]

In general, +1.

On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:
> (Still need to think about Robert's comment about lack of error context.)

Would adding the name of the GUC be sufficient?

ereport(ERROR,
(errmsg("could not build %s", guc_name),
 errdetail("string ends unexpectedly after escape 
character \"%%\"")));

> + * A value may be NULL.  If the corresponding placeholder is found in the
> + * input string, the whole function returns NULL.

This appears to be carried over from BuildRestoreCommand(), and AFAICT it
is only necessary because pg_rewind doesn't support %r in restore_command.
IMHO this behavior is counterintuitive and possibly error-prone and should
result in an ERROR instead.  Since pg_rewind is the only special case, it
could independently check for %r before building the command.

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

Re: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

[David Rowley]

On Tue, 3 Jan 2023 at 19:59, Ankit Kumar Pandey  wrote:
>
>
> On 03/01/23 08:38, David Rowley wrote:
> > Do you actually have a need for this or are you just trying to tick
> > off some TODO items?
> >
> I would say Iatter but reason I picked it up was more on side of
> learning optimizer better.

I think it's better you leave this then. I think if someone comes
along and demonstrates the feature's usefulness and can sell us having
it so we can easily enable it by GUC then maybe that's the time to
consider it. I don't think ticking off a TODO item is reason enough.

> Also from the thread,
>
> https://www.postgresql.org/message-id/20120821.121611.501104647612634419.t-is...@sraoss.co.jp
>
> > +1. It would also be popular with our academic users.
> >
> There could be potential for this as well.

I think the argument is best coming from someone who'll actually use it.

David

Re: pgsql: Delay commit status checks until freezing executes.

[Peter Geoghegan]

(Pruning -committers from the list, since cross-posting to -hackers
resulted in this being held up for moderation.)

On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan  wrote:
>
> On Tue, Jan 3, 2023 at 4:54 PM Andres Freund  wrote:
> > There's some changes from TransactionIdDidCommit() to 
> > !TransactionIdDidAbort()
> > that don't look right to me. If the server crashed while xid X was
> > in-progress, TransactionIdDidCommit(X) will return false, but so will
> > TransactionIdDidAbort(X). So besides moving when the check happens you also
> > changed what's being checked in a more substantial way.
>
> I did point this out on the thread. I made this change with the
> intention of making the check more robust. Apparently this was
> misguided.
>
> Where is the behavior that you describe documented, if anywhere?

When the server crashes, and we have a problem case, what does
TransactionLogFetch()/TransactionIdGetStatus() (which are the guts of
both TransactionIdDidCommit and TransactionIdDidAbort) report about
the XID?

> > Also, why did you change when MarkBufferDirty() happens? Previously it
> > happened before we modify the page contents, now after. That's probably fine
> > (it's the order suggested in transam/README), but seems like a mighty subtle
> > thing to change at the same time as something unrelated, particularly 
> > without
> > even mentioning it?
>
> I changed it because the new order is idiomatic. I didn't think that
> this was particularly worth mentioning, or even subtle. The logic from
> heap_execute_freeze_tuple() only performs simple in-place
> modifications.

I'm including this here because presumably -hackers will have missed
it due to the moderation hold-up issue.


--
Peter Geoghegan

Re: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

[Vladimir Churyukin]

As an end user that spends a lot of time optimizing pretty complicated
queries, I'd say that something like this could be useful.
Right now the optimizer is mostly a black box. Why it chooses one plan or
the other, it's a mystery. I have some general ideas about that,
and I can even read and sometimes debug optimizer's code to dig deeper
(although it's not always possible to reproduce the same behavior as in the
production system anyway).
I'm mostly interested to find where exactly the optimizer was wrong and
what would be the best way to fix it. Currently Postgres is not doing a
great job in that department.
EXPLAIN output can tell you about mispredictions, but the logic of choosing
particular plans is still obscure, because the reasons for optimizer's
decisions are not visible.
If configuring OPTIMIZER_DEBUG through GUC can help with that, I think it
would be a useful addition.
Now, that's general considerations, I'm not somebody who actually uses
OPTIMIZER_DEBUG regularly (but maybe I would if it's accessible
through GUC),
I'm just saying that is an area where improvements would be very much
welcomed.

-Vladimir

On Tue, Jan 3, 2023 at 4:57 PM David Rowley  wrote:

> On Tue, 3 Jan 2023 at 19:59, Ankit Kumar Pandey 
> wrote:
> >
> >
> > On 03/01/23 08:38, David Rowley wrote:
> > > Do you actually have a need for this or are you just trying to tick
> > > off some TODO items?
> > >
> > I would say Iatter but reason I picked it up was more on side of
> > learning optimizer better.
>
> I think it's better you leave this then. I think if someone comes
> along and demonstrates the feature's usefulness and can sell us having
> it so we can easily enable it by GUC then maybe that's the time to
> consider it. I don't think ticking off a TODO item is reason enough.
>
> > Also from the thread,
> >
> >
> https://www.postgresql.org/message-id/20120821.121611.501104647612634419.t-is...@sraoss.co.jp
> >
> > > +1. It would also be popular with our academic users.
> > >
> > There could be potential for this as well.
>
> I think the argument is best coming from someone who'll actually use it.
>
> David
>
>
>

Re: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

[David Rowley]

On Wed, 4 Jan 2023 at 16:15, Vladimir Churyukin  wrote:
> As an end user that spends a lot of time optimizing pretty complicated 
> queries, I'd say that something like this could be useful.

I think we really need to at least see that it *is* useful, not that
it *could be* useful.  For example, as an end user, you might not find
it great that the output is sent to stdout rather than to the window
that you execute the query in.

>From what I can see here, the motivation to make this a useful feature
is backwards from what is normal. I think if you're keen to see a
feature that allows you better visibility into rejected paths then you
need to prove this is it rather than speculating that it might be
useful.

There was a bit of work being done in [1] with the end goal of having
the ability for add_path to call a hook function before it outright
rejects a path.  Maybe that would be a better place to put this and
then write some contrib module that provides some extended output in
EXPLAIN. That might require some additional fields so that we could
carry forward additional information that we'd like to show in
EXPLAIN. I imagine it's not ok just to start writing result lines in
the planner. The EXPLAIN format must be considered too and explain.c
seems like the place that should be done. add_path might need to
become a bit more verbose about the reason it rejected a certain path
for this to be useful.

David

[1] https://commitfest.postgresql.org/39/3599/

Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?

[Thomas Munro]

On Mon, Jan 2, 2023 at 8:38 AM Thomas Munro  wrote:
> On Sat, Dec 31, 2022 at 6:36 PM Noah Misch  wrote:
> > On Sat, Dec 31, 2022 at 10:06:53AM +1300, Thomas Munro wrote:
> > > Idea #8 is a realisation that twisting oneself into a pretzel to avoid
> > > having to change the regexp code or its REG_CANCEL control flow may be
> > > a bit silly.  If the only thing it really needs to do is free some
> > > memory, maybe the regexp module should provide a function that frees
> > > everything that is safe to call from our rcancelrequested callback, so
> > > we can do so before we longjmp back to Kansas.  Then the REG_CANCEL
> > > code paths would be effectively unreachable in PostgreSQL.  I don't
> > > know if it's better or worse than your idea #6, "use palloc instead,
> > > it already has garbage collection, duh", but it's a different take on
> > > the same realisation that this is just about free().
> >
> > PG_TRY() isn't free, so it's nice that (6) doesn't add one.  If (6) fails in
> > some not-yet-revealed way, (8) could get more relevant.
> >
> > > I guess idea #6 must be pretty easy to try: just point that MALLOC()
> > > macro to palloc(), and do a plain old CFI() in rcancelrequested().
>
> It's not quite so easy: in RE_compile_and_cache we construct objects
> with arbitrary cache-managed lifetime, which suggests we need a cache
> memory context, but we could also fail mid construction, which
> suggests we'd need a dedicated per-regex object memory context that is
> made permanent with the MemoryContextSetParent() trick (as we see
> elsewhere for cached things that are constructed by code that might
> throw), ...

Here's an experiment-grade attempt at idea #6 done that way, for
discussion.  You can see how much memory is wasted by each regex_t,
which I guess is probably on the order of a couple of hundred KB if
you use all 32 regex cache slots using ALLOCSET_SMALL_SIZES as I did
here:

postgres=# select 'x' ~ 'hello world .*';
-[ RECORD 1 ]
?column? | f

postgres=# select * from pg_backend_memory_contexts where name =
'RegexpMemoryContext';
-[ RECORD 1 ]-+-
name  | RegexpMemoryContext
ident | hello world .*
parent| RegexpCacheMemoryContext
level | 2
total_bytes   | 13376
total_nblocks | 5
free_bytes| 5144
free_chunks   | 8
used_bytes| 8232

There's some more memory allocated in regc_pg_locale.c with raw
malloc() that could probably benefit from a pallocisation just to be
able to measure it, but I didn't touch that here.

The recovery conflict patch itself is unchanged, except that I removed
the claim in the commit message that this would be back-patched.  It's
pretty clear that this would need to spend a decent amount of time on
master only.
From ee8eafb249368b889308fa23704f2a34a575b254 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Wed, 4 Jan 2023 14:15:40 +1300
Subject: [PATCH v4 1/2] Use MemoryContext API for regexp memory management.

Previously, regex_t objects' memory was managed using malloc() and
free() directly.  Since regexp compilation can take same time, regcomp()
would periodically call a callback function that would check for query
cancelation.  That design assumed that asynchronous query cancelation
could be detected by reading flags set by signal handlers, and that the
flags were a reliable indication that a later CHECK_FOR_INTERRUPTS()
would exit or throw an error.  This allowed the code to free memory
before aborting.

A later commit will refactor the recovery conflict interrupt system, to
move its logic out of signal handlers, because it is not
async-signal-safe (among other problems).  That would break the above
assumption, so we need another approach to memory cleanup.

Switch to using palloc(), the standard mechanism for garbage collection
in PostgreSQL.  Since regex_t objects have to survive across transaction
boundaries in a cache, introduce RegexpCacheMemoryContext.  Since
partial regex_t objects need to be cleaned up on failure to compile due
to interruption, also give each regex_t its own context.  It is
re-parented to the longer living RegexpCacheMemoryContext only if
compilation is successful, following a pattern seen elsewhere in the
tree.

XXX Experimental, may contain nuts
---
 src/backend/regex/regcomp.c| 14 +++
 src/backend/utils/adt/regexp.c | 70 --
 src/include/regex/regcustom.h  |  6 +--
 3 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/backend/regex/regcomp.c b/src/backend/regex/regcomp.c
index bb8c240598..c0f8e77b49 100644
--- a/src/backend/regex/regcomp.c
+++ b/src/backend/regex/regcomp.c
@@ -2471,17 +2471,17 @@ rfree(regex_t *re)
 /*
  * rcancelrequested - check for external request to cancel regex operation
  *
- * Return nonzero to fail the operation with error code REG_CANCEL,
- * zero to keep going
- *
- * The current implementation is Postgres-specific.  If we ever get around
- * to splitting the regex code out as a standalone library, 

Re: pgsql: Delay commit status checks until freezing executes.

[Andres Freund]

Hi,

On 2023-01-03 17:54:37 -0800, Peter Geoghegan wrote:
> (Pruning -committers from the list, since cross-posting to -hackers
> resulted in this being held up for moderation.)

I still think these moderation rules are deeply unhelpful...


> On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan  wrote:
> >
> > On Tue, Jan 3, 2023 at 4:54 PM Andres Freund  wrote:
> > > There's some changes from TransactionIdDidCommit() to 
> > > !TransactionIdDidAbort()
> > > that don't look right to me. If the server crashed while xid X was
> > > in-progress, TransactionIdDidCommit(X) will return false, but so will
> > > TransactionIdDidAbort(X). So besides moving when the check happens you 
> > > also
> > > changed what's being checked in a more substantial way.
> >
> > I did point this out on the thread. I made this change with the
> > intention of making the check more robust. Apparently this was
> > misguided.
> >
> > Where is the behavior that you describe documented, if anywhere?

I don't know - I think there's a explicit comment somewhere, but I couldn't
find it immediately. There's a bunch of indirect references to in in
heapam_visibility.c, with comments like "it must have aborted or
crashed".

The reason for the behaviour is that we do not have any mechanism for going
through the clog and aborting all in-progress-during-crash transactions. So
we'll end up with the clog for all in-progress-during-crash transaction being
zero / TRANSACTION_STATUS_IN_PROGRESS.

IMO it's almost always wrong to use TransactionIdDidAbort().


> When the server crashes, and we have a problem case, what does
> TransactionLogFetch()/TransactionIdGetStatus() (which are the guts of
> both TransactionIdDidCommit and TransactionIdDidAbort) report about
> the XID?

Depends a bit on the specifics, but mostly TRANSACTION_STATUS_IN_PROGRESS.



> > > Also, why did you change when MarkBufferDirty() happens? Previously it
> > > happened before we modify the page contents, now after. That's probably 
> > > fine
> > > (it's the order suggested in transam/README), but seems like a mighty 
> > > subtle
> > > thing to change at the same time as something unrelated, particularly 
> > > without
> > > even mentioning it?
> >
> > I changed it because the new order is idiomatic. I didn't think that
> > this was particularly worth mentioning, or even subtle. The logic from
> > heap_execute_freeze_tuple() only performs simple in-place
> > modifications.

I think changes in how WAL logging is done are just about always worth
mentioning in a commit message...

Greetings,

Andres Freund

Re: Todo: Teach planner to evaluate multiple windows in the optimal order

[David Rowley]

On Wed, 4 Jan 2023 at 03:11, Ankit Kumar Pandey  wrote:
> #2. If order by clause in the Window function is superset of order by in query
>
> explain analyze select a,row_number() over (order by a,b,c),count(*) over 
> (order by a,b) from abcd order by a;
>
>   QUERY PLAN
> --
>  WindowAgg  (cost=39.27..64.27 rows=625 width=28) (actual time=1.089..3.020 
> rows=625 loops=1)
>->  WindowAgg  (cost=39.27..53.34 rows=625 width=20) (actual 
> time=1.024..1.635 rows=625 loops=1)
>  ->  Sort  (cost=39.27..40.84 rows=625 width=12) (actual 
> time=1.019..1.084 rows=625 loops=1)
>Sort Key: a, b, c
>Sort Method: quicksort  Memory: 54kB
>->  Seq Scan on abcd  (cost=0.00..10.25 rows=625 width=12) 
> (actual time=0.023..0.265 rows=625 loops=1)
>  Planning Time: 0.071 ms
>  Execution Time: 3.156 ms
> (8 rows)
>
> No, additional sort is needed to be performed in this case, as you referred.

It looks like that works by accident. I see no mention of this either
in the comments or in [1].  What seems to be going on is that
common_prefix_cmp() is coded in such a way that the WindowClauses end
up ordered by the highest tleSortGroupRef first, resulting in the
lowest order tleSortGroupRefs being the last WindowAgg to be
processed.  We do transformSortClause() before
transformWindowDefinitions(), this is where the tleSortGroupRef
indexes are assigned, so the ORDER BY clause will have a lower
tleSortGroupRef than the WindowClauses.

If we don't have one already, then we should likely add a regression
test that ensures that this remains true.  Since it does not seem to
be documented in the code anywhere, it seems like something that could
easily be overlooked if we were to ever refactor that code.

I just tried moving the calls to transformWindowDefinitions() so that
they come before transformSortClause() and our regression tests still
pass.  That's not great.

With that change, the following query has an additional sort for the
ORDER BY clause which previously wasn't done.

explain select a,b,c,row_number() over (order by a) rn1, row_number()
over(partition by b) rn2, row_number() over (order by c) from abc
order by b;

David

[1] 
https://www.postgresql.org/message-id/flat/124A7F69-84CD-435B-BA0E-2695BE21E5C2%40yesql.se

Re: wake up logical workers after ALTER SUBSCRIPTION

[Amit Kapila]

On Tue, Jan 3, 2023 at 11:51 PM Nathan Bossart  wrote:
>
> On Tue, Jan 03, 2023 at 11:43:59AM +0530, Amit Kapila wrote:
> > On Wed, Dec 7, 2022 at 11:42 PM Nathan Bossart  
> > wrote:
> >> After sleeping on this, I think we can do better.  IIUC we can simply check
> >> for AllTablesyncsReady() at the end of process_syncing_tables_for_apply()
> >> and wake up the logical replication workers (which should just consiѕt of
> >> setting the current process's latch) if we are ready for two_phase mode.
> >
> > How just waking up will help with two_phase mode? For that, we need to
> > restart the apply worker as we are doing at the beginning of
> > process_syncing_tables_for_apply().
>
> Right.  IIRC waking up causes the apply worker to immediately call
> process_syncing_tables_for_apply() again, which will then proc_exit(0) as
> appropriate.
>

But we are already in apply worker and performing
process_syncing_tables_for_apply(). This means the apply worker is not
waiting/sleeping, so what exactly are we trying to wake up?

>  It might be possible to move the restart logic to the end of
> process_syncing_tables_for_apply() to avoid this extra wakeup.  WDYT?
>

I am not sure if I understand the problem you are trying to solve with
this part of the patch. Are you worried that after we mark some of the
relation's state as READY, all the table syncs are in the READY state
but we will not immediately try to check the two_pahse stuff and
probably the apply worker may sleep before the next time it invokes
process_syncing_tables_for_apply()? If so, we probably also need to
ensure that table_states_valid is marked false probably via
invalidations so that we can get the latest state and then perform
this check. I guess if we can do that then we can directly move the
restart logic to the end.

-- 
With Regards,
Amit Kapila.