Re: BF mamba failure

[Michael Paquier]

On Wed, Jun 12, 2024 at 08:00:00PM +0300, Alexander Lakhin wrote:
> With extra logging added, I see the following events happening:
> 1) pgstat_rc_1.setup calls pgstat_create_replslot(), gets
>   ReplicationSlotIndex(slot) = 0 and calls
>   pgstat_get_entry_ref_locked(PGSTAT_KIND_REPLSLOT, InvalidOid, 0, 0).
> 
> 2) pgstat_rc_1.s0_get_changes executes pg_logical_slot_get_changes(...)
>   and then calls pgstat_gc_entry_refs on shmem_exit() ->
>   pgstat_shutdown_hook() ...;
>   with the sleep added inside pgstat_release_entry_ref, this backend waits
>   after decreasing entry_ref->shared_entry->refcount to 0.
> 
> 3) pgstat_rc_1.stop removes the replication slot.
> 
> 4) pgstat_rc_2.setup calls pgstat_create_replslot(), gets
>   ReplicationSlotIndex(slot) = 0 and calls
>   pgstat_get_entry_ref_locked(PGSTAT_KIND_REPLSLOT, InvalidOid, 0, 0),
>   which leads to the call pgstat_reinit_entry(), which increases refcount
>   for the same shared_entry as in (1) and (2), and then to the call
>   pgstat_acquire_entry_ref(), which increases refcount once more.
> 
> 5) the backend 2 reaches
> Assert(pg_atomic_read_u32(_ref->shared_entry->refcount) == 0),
>   which fails due to refcount = 2.

Thanks for the details.

So this comes down to the point that we offer no guarantee that the
stats entry a backend sees at shutdown is the same as the one it wants
to clean up.  That's the same problem as what Floris has reported
here, with an OID wraparound and tables to get the same hash key.
That can happen for all stats kinds:
https://www.postgresql.org/message-id/b14ae28029f64757bb64613be2549...@optiver.com

I don't think that this is going to fly far except if we introduce a
concept of "generation" or "age" in the stats entries.  The idea is
simple: when a stats entry is reinitialized because of a drop,
increment a counter to tell that this is a new generation, and keep
track of it in *both* PgStat_EntryRef (local backend reference to the
shmem stats entry) *and* PgStatShared_HashEntry (the central one).
When releasing an entry, if we know that the shared entry we are
pointing at is not of the same generation as the local reference, it
means that the entry has been reused for something else with the same
hash key, so give up.  It should not be that invasive, still it means
ABI breakage in the two pgstats internal structures I am mentioning,
which is OK for a backpatch as this stuff is internal.  On top of
that, this problem means that we can silently and randomly lose stats,
which is not cool.

Note that Noah has been working on a better integration of injection
points with isolation tests.  We could reuse that here to have a test
case, with an injection point waiting around the pg_usleep() you have
hardcoded:
https://www.postgresql.org/message-id/20240614003549.c2.nmi...@google.com

I'll try to give it a go on Monday.
--
Michael


signature.asc
Description: PGP signature

Re: SQL function which allows to distinguish a server being in point in time recovery mode and an ordinary replica

[Michael Paquier]

On Thu, Jun 13, 2024 at 09:07:42PM +0300, m.litsa...@postgrespro.ru wrote:
> Hi!
> 
> Michael,
> I have fixed the patches according to your comments.
> 
> > merge both local variables into a single bits32 store?
> This is done in v3-0001-Standby-mode-requested.patch
> 
> > Then this could be used with a function that returns a
> > text[] array with all the states retrieved?
> Placed this in the v3-0002-Text-array-sql-wrapper.patch
> 
> > The refactoring pieces and the function pieces should be split, for
> > clarity.
> Sure. I also added the third patch with some tests. Perhaps it would be
> usefull.

+ *--XLR_PROMOTE_IS_TRIGGERED indicates if a standby promotion
+ *has been triggered. Protected by info_lck.
+ *
+ * --XLR_STANDBY_MODE_REQUESTED indicates if we're in a standby 
mode
+ *at start, while recovery mode is on. No info_lck protection.
+ *
+ *and can be extended in future.

This comment is incorrect for XLR_STANDBY_MODE_REQUESTED?  A startup
we would unlikely be in standby mode, most likely in crash recovery,
then switch to standby mode.

-LocalPromoteIsTriggered = XLogRecoveryCtl->SharedPromoteIsTriggered;
+LocalRecoveryDataFlags &= ~XLR_PROMOTE_IS_TRIGGERED;
+LocalRecoveryDataFlags |=
+(XLogRecoveryCtl->SharedRecoveryDataFlags & XLR_PROMOTE_IS_TRIGGERED)

Are these complications really needed?  All these flags are false,
then switched to true.  true -> false is not possible.

 StandbyModeRequested = true;
 ArchiveRecoveryRequested = true;
+XLogRecoveryCtl->SharedRecoveryDataFlags |= XLR_STANDBY_MODE_REQUESTED;

Shouldn't STANDBY_MODE be only used in the local flag, as well as an
ARCHIVE_RECOVERY_REQUESTED?  It looks like this could push a bit more
forward the removal of more of these booleans, with a bit more work..

 return (LocalRecoveryDataFlags & XLR_HOT_STANDBY_ACTIVE);
 }
 
+
 /*
Some noise lying around.

+   /* Returns bit array as Datum */
+   txt_arr = construct_array_builtin(flags, cnt, TEXTOID);

Yep, that's the correct way to do it.

+is($ret_mode_primary, '{}', "master is not a replica"); 

The test additions are welcome.  Note that we avoid the word "master",
see 229f8c219f8f.
--
Michael


signature.asc
Description: PGP signature

Re: Doc: fix a description regarding WAL summarizer on glossary page

[Masahiro Ikeda]

On 2024-06-13 17:01, Michael Paquier wrote:
On Thu, Jun 13, 2024 at 07:24:10AM +, masahiro.ik...@nttdata.com 
wrote:

Thanks for your response. Please take a look at the attached patch.
I've confirmed that it passes all tests.


Thanks for the patch.  Will check and apply.


Thanks for applying.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Re: Should consider materializing the cheapest inner path in consider_parallel_nestloop()

[Tender Wang]

Hi Robert,

Since this patch had been reviewed at PgConf.dev Patch Review
Workshop.  And I have updated
the patch according to the review advice. Now there are no others to
comment this patch.
The status of this patch on commitfest have stayed "need review" for a long
time.
I want to know if it is ready to move to the next status "Ready for
commiter".

Thanks.

-- 
Tender Wang

Re: Remove dependence on integer wrapping

[Joseph Koshakow]

On Thu, Jun 13, 2024 at 10:48 PM Joseph Koshakow  wrote:
>I've attached
>v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
>overflow checks and tests. I didn't address the float multiplication
>because I didn't see any helper methods in int.h. I did some some
>useful helpers in float.h, but they raise an error directly instead
>of returning a bool. Would those be appropriate for use with the
>money type? If not I can refactor out the inner parts into a new method
>that returns a bool.

>v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
>just incremented the version number.

Oops I left a careless mistake in that last patch, my apologies. It's
fixed in the attached patches.

Thanks,
Joe Koshakow
From c54925ef698d37d968f138585141d308fe1acacc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Thu, 13 Jun 2024 22:39:25 -0400
Subject: [PATCH 2/2] Handle overflow in money arithmetic

---
 src/backend/utils/adt/cash.c| 40 +++--
 src/test/regress/expected/money.out | 29 +
 src/test/regress/sql/money.sql  | 16 
 3 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index f6f095a57b..e5e51aefbc 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -617,7 +617,10 @@ cash_pl(PG_FUNCTION_ARGS)
 	Cash		c2 = PG_GETARG_CASH(1);
 	Cash		result;
 
-	result = c1 + c2;
+	if (pg_add_s64_overflow(c1, c2, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 
 	PG_RETURN_CASH(result);
 }
@@ -633,7 +636,10 @@ cash_mi(PG_FUNCTION_ARGS)
 	Cash		c2 = PG_GETARG_CASH(1);
 	Cash		result;
 
-	result = c1 - c2;
+	if (pg_sub_s64_overflow(c1, c2, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 
 	PG_RETURN_CASH(result);
 }
@@ -770,7 +776,10 @@ cash_mul_int8(PG_FUNCTION_ARGS)
 	int64		i = PG_GETARG_INT64(1);
 	Cash		result;
 
-	result = c * i;
+	if (pg_mul_s64_overflow(c, i, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
@@ -785,7 +794,10 @@ int8_mul_cash(PG_FUNCTION_ARGS)
 	Cash		c = PG_GETARG_CASH(1);
 	Cash		result;
 
-	result = i * c;
+	if (pg_mul_s64_overflow(i, c, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
@@ -820,7 +832,10 @@ cash_mul_int4(PG_FUNCTION_ARGS)
 	int32		i = PG_GETARG_INT32(1);
 	Cash		result;
 
-	result = c * i;
+	if (pg_mul_s64_overflow(c, (int64) i, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
@@ -835,7 +850,10 @@ int4_mul_cash(PG_FUNCTION_ARGS)
 	Cash		c = PG_GETARG_CASH(1);
 	Cash		result;
 
-	result = i * c;
+	if (pg_mul_s64_overflow((int64) i, c, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
@@ -872,7 +890,10 @@ cash_mul_int2(PG_FUNCTION_ARGS)
 	int16		s = PG_GETARG_INT16(1);
 	Cash		result;
 
-	result = c * s;
+	if (pg_mul_s64_overflow(c, (int64) s, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
@@ -886,7 +907,10 @@ int2_mul_cash(PG_FUNCTION_ARGS)
 	Cash		c = PG_GETARG_CASH(1);
 	Cash		result;
 
-	result = s * c;
+	if (pg_mul_s64_overflow((int64) s, c, ))
+		ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("money out of range")));
 	PG_RETURN_CASH(result);
 }
 
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out
index 7fd4e31804..950e6410a4 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -528,3 +528,32 @@ SELECT '-92233720368547758.08'::money::numeric;
  -92233720368547758.08
 (1 row)
 
+-- Test overflow checks
+SELECT '92233720368547758.07'::money + '0.01'::money;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money - '0.01'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int8;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::int8;
+ERROR:  money out of range
+SELECT 2::int8 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::int8 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int4;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::int4;
+ERROR:  money out of range
+SELECT 2::int4 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::int4 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int2;
+ERROR:  money out of range
+SELECT 

Re: Remove dependence on integer wrapping

[Joseph Koshakow]

On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin 
wrote:
>
>Let me remind you of bug #18240. Yes, that was about float8, but with
>-ftrapv we can get into the trap with:
>SELECT 1_000_000_000::money * 1_000_000_000::int;
>server closed the connection unexpectedly

Interesting, it looks like there's no overflow handling of any money
arithmetic. I've attached
v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
overflow checks and tests. I didn't address the float multiplication
because I didn't see any helper methods in int.h. I did some some
useful helpers in float.h, but they raise an error directly instead
of returning a bool. Would those be appropriate for use with the
money type? If not I can refactor out the inner parts into a new method
that returns a bool.

v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.

>Also there are several trap-producing cases with date types:
>SELECT to_date('1', 'CC');
>SELECT to_timestamp('10,999', 'Y,YYY');
>SELECT make_date(-2147483648, 1, 1);
>
>And one more with array...
>CREATE TABLE t (ia int[]);
>INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');

I'll try and get patches to address these too in the next couple of
weeks unless someone beats me to it.

>I think it's not the whole iceberg too.

+1

Thanks,
Joe Koshakow
From 31e8de30a82e60151848439143169e562bc848a3 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/2] Remove dependence on integer wrapping

This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
 src/backend/utils/adt/cash.c   |   7 +-
 src/backend/utils/adt/numeric.c|   5 +-
 src/backend/utils/adt/numutils.c   |  34 ---
 src/backend/utils/adt/timestamp.c  |  28 +-
 src/include/common/int.h   | 105 +
 src/interfaces/ecpg/pgtypeslib/timestamp.c |  11 +--
 src/test/regress/expected/timestamp.out|  13 +++
 src/test/regress/expected/timestamptz.out  |  13 +++
 src/test/regress/sql/timestamp.sql |   4 +
 src/test/regress/sql/timestamptz.sql   |   4 +
 10 files changed, 169 insertions(+), 55 deletions(-)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
 
 	if (value < 0)
 	{
-		/* make the amount positive for digit-reconstruction loop */
-		value = -value;
+		/*
+		 * make the amount positive for digit-reconstruction loop, we can
+		 * leave INT64_MIN unchanged
+		 */
+		pg_neg_s64_overflow(value, );
 		/* set up formatting data */
 		signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
 		sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
 
 	/* int64 can require at most 19 decimal digits; add one for safety */
 	alloc_var(var, 20 / DEC_DIGITS);
+	uval = pg_abs_s64(val);
 	if (val < 0)
 	{
 		var->sign = NUMERIC_NEG;
-		uval = -val;
 	}
 	else
 	{
 		var->sign = NUMERIC_POS;
-		uval = val;
 	}
 	var->dscale = 0;
 	if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
 	 * Now we can proceed with the multiplications.
 	 */
 	neg = (exp < 0);
-	mask = abs(exp);
+	mask = pg_abs_s32(exp);
 
 	init_var(_prod);
 	set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 
+#include "common/int.h"
 #include "port/pg_bitutils.h"
 #include "utils/builtins.h"
 
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
 	uint16		tmp = 0;
 	bool		neg = false;
 	unsigned char digit;
+	int16		result;
 
 	/*
 	 * The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+		if (pg_neg_u16_overflow(tmp, ))
 			goto out_of_range;
-		return -((int16) tmp);
+		return result;
 	}
 
 	if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+		if (pg_neg_u16_overflow(tmp, ))
 			goto out_of_range;
-		return -((int16) 

Re: race condition in pg_class

[Noah Misch]

On Fri, Jun 14, 2024 at 09:58:59AM +0900, Michael Paquier wrote:
> Looking at inplace031-inj-wait-event..
> 
> The comment at the top of GetWaitEventCustomNames() requires an
> update, still mentioning extensions.

Thanks.  Fixed locally.

> GetWaitEventCustomIdentifier() is incorrect, and should return
> "InjectionPoint" in the default case of this class name, no?

I intentionally didn't provide a default event ID for InjectionPoint.
PG_WAIT_EXTENSION needs a default case for backward compatibility, if nothing
else.  For this second custom type, it's needless complexity.  The value
0x0B00U won't just show up like PG_WAIT_EXTENSION does.
GetLWLockIdentifier() also has no default case.  How do you see it?

> I would
> just pass the classID to GetWaitEventCustomIdentifier().

As you say, that would allow eventId==0 to raise "could not find custom wait
event" for PG_WAIT_INJECTIONPOINT instead of wrongly returning "Extension".
Even if 0x0B00U somehow does show up, having pg_stat_activity report
"Extension" instead of an error, in a developer test run, feels unimportant to
me.

> It is suboptimal to have pg_get_wait_events() do two scans of
> WaitEventCustomHashByName.  Wouldn't it be better to do a single scan,
> returning a set of (class_name,event_name) fed to the tuplestore of
> this SRF?

Micro-optimization of pg_get_wait_events() doesn't matter.  I did consider
that or pushing more of the responsibility into wait_events.c, but I
considered it on code repetition grounds, not performance grounds.

>  uint32
>  WaitEventExtensionNew(const char *wait_event_name)
>  {
> + return WaitEventCustomNew(PG_WAIT_EXTENSION, wait_event_name);
> +}
> +
> +uint32
> +WaitEventInjectionPointNew(const char *wait_event_name)
> +{
> + return WaitEventCustomNew(PG_WAIT_INJECTIONPOINT, wait_event_name);
> +}
> 
> Hmm.  The advantage of two routines is that it is possible to control
> the class IDs allowed to use the custom wait events.  Shouldn't the
> second routine be documented in xfunc.sgml?

The patch added to xfunc.sgml an example of using it.  I'd be more inclined to
delete the WaitEventExtensionNew() docbook documentation than to add its level
of detail for WaitEventInjectionPointNew().  We don't have that kind of
documentation for most extension-facing C functions.

Re: jsonpath: Missing Binary Execution Path?

[Chapman Flack]

On 06/13/24 22:16, David E. Wheeler wrote:
> But even inside filters I don’t understand why &&, ||, at least,
> currently only work if their operands are predicate expressions.
> Seems weird; and your notes above suggest that rule applies only to !,
> which makes slightly more sense.

It's baked right into the standard grammar: || can only have a
 on its right and a 
on its left.

&& can only have a  on its right and a
 on its left.

The case for ! is even more limiting: it can't be applied to anything
but a . That can be either the exists predicate,
or, any other  but wrapped in parentheses.

The language seems sort of gappy in the same way XPath 1.0 was. XPath 2.0
became much more consistent and conceptually unified, only by that time,
XML was old school, and JSON was cool, and apparently started inventing
a path language.

Regards,
-Chap

Re: jsonpath: Missing Binary Execution Path?

[David E. Wheeler]

On Jun 13, 2024, at 21:58, Chapman Flack  wrote:

 david=# select jsonb_path_query('1', '$ >= 1');
>>> 
>>> Good point. I can't either. No way I can see to parse that as
>>> a .
>> 
>> Whether we note it as non-standard or not is an open question then, but it
>> does work and opens up a documentation question.
> 
> Does the fact that it does work raise any potential concern that our
> grammar is nonconformant in some way that could present a headache
> somewhere else, or down the road with a later standard edition?

I believe this case is already covered in the docs as a Postgres-specific 
feature: predicate path expressions.

But even inside filters I don’t understand why &&, ||, at least, currently only 
work if their operands are predicate expressions. Seems weird; and your notes 
above suggest that rule applies only to !, which makes slightly more sense.

D

Re: jsonpath: Missing Binary Execution Path?

[David G. Johnston]

On Thursday, June 13, 2024, Chapman Flack  wrote:

> On 06/13/24 21:46, David G. Johnston wrote:
> >>> david=# select jsonb_path_query('1', '$ >= 1');
> >>
> >> Good point. I can't either. No way I can see to parse that as
> >> a .
> >
> > Whether we note it as non-standard or not is an open question then, but
> it
> > does work and opens up a documentation question.
>
> Does the fact that it does work raise any potential concern that our
> grammar is nonconformant in some way that could present a headache
> somewhere else, or down the road with a later standard edition?
>

This isn’t new in v17 nor, to my knowledge, has the behavior changed, so I
think we just need to live with whatever, likely minimal, chance of
headache there is.

I don’t get why the outcome of a boolean producing operation isn’t just
generally allowed to be produced, and would hope the standard would move
toward allowing that across the board, and in doing so end up matching what
we already have implemented.

David J.

Re: jsonpath: Missing Binary Execution Path?

[Chapman Flack]

On 06/13/24 21:46, David G. Johnston wrote:
>>> david=# select jsonb_path_query('1', '$ >= 1');
>>
>> Good point. I can't either. No way I can see to parse that as
>> a .
> 
> Whether we note it as non-standard or not is an open question then, but it
> does work and opens up a documentation question.

Does the fact that it does work raise any potential concern that our
grammar is nonconformant in some way that could present a headache
somewhere else, or down the road with a later standard edition?

Regards,
-Chap

Re: Shouldn't jsonpath .string() Unwrap?

[Chapman Flack]

On 06/13/24 18:45, David E. Wheeler wrote:
> On Jun 13, 2024, at 3:53 PM, Andrew Dunstan  wrote:
> 
>> Hmm. You might be right. Many of these items have this code, but the 
>> string() branch does not:
>> if (unwrap && JsonbType(jb) == jbvArray)
>>return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
>>false);
> 
> Exactly, would be pretty easy to add. I can work up a patch this weekend.

My opinion is yes, that should be done. 9.46, umm, General
Rule 11 g ii 6) A) says just "if MODE is lax and  is not
type or size, then let BASE be Unwrap(BASE)." No special exemption
there for string(), nor further below at C) XV) for the operation
of string().

Regards,
-Chap

Re: jsonpath: Missing Binary Execution Path?

[David G. Johnston]

On Thursday, June 13, 2024, Chapman Flack  wrote:

> On 06/13/24 21:24, David G. Johnston wrote:
> > I'm content that the operators in the 'filter operators' table need to be
> > within filter but then I cannot reconcile why this example worked:
> >
> > david=# select jsonb_path_query('1', '$ >= 1');
>
> Good point. I can't either. No way I can see to parse that as
> a .
>


Whether we note it as non-standard or not is an open question then, but it
does work and opens up a documentation question.  It seems like it needs to
appear in table T9.50.  Whether it also should appear in T9.51 is the
question.  It seems like anything in T9.50 is allowed in a filter while the
stuff in T9.51 should be limited to those things only allowed in a filter.
Which suggests moving it from T9.51 to T9.50

David J.

Re: jsonpath: Missing Binary Execution Path?

[Chapman Flack]

On 06/13/24 21:24, David G. Johnston wrote:
> I'm content that the operators in the 'filter operators' table need to be
> within filter but then I cannot reconcile why this example worked:
> 
> david=# select jsonb_path_query('1', '$ >= 1');

Good point. I can't either. No way I can see to parse that as
a .

Regards,
-Chap

Re: jsonpath: Missing Binary Execution Path?

[David G. Johnston]

On Thu, Jun 13, 2024 at 6:10 PM Chapman Flack  wrote:

> On 06/13/24 16:43, David E. Wheeler wrote:
> > Paging Mr. Eisentraut!
>
> I'm not Mr. Eisentraut, but I have at last talked my way into some
> access to the standard, so ...
>
> Note 487 emphasizes that JSON path predicates "are not expressions;
> instead they form a separate language that can only be invoked within
> a ".
>
> The only operators usable in a general expression (that is, a
>  are binary + - and binary * / % and unary + -
> over a .
>
> Inside a filter, you get to use a . That's where
> you can use ! and && and ||. But ! can only be applied to a
> : either a ,
> or any other  wrapped in parentheses.
>
> On 06/13/24 11:32, David E. Wheeler wrote:
> > david=# select jsonb_path_query('true', '$ && $');
> > david=# select jsonb_path_query('true', '$.boolean() && $.boolean()');
>
> Those don't work because, as you recognized, they're not inside filters.
>

I'm content that the operators in the 'filter operators' table need to be
within filter but then I cannot reconcile why this example worked:

david=# select jsonb_path_query('1', '$ >= 1');
 jsonb_path_query
--
 true
(1 row)

David J.

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Masahiko Sawada]

On Fri, Jun 14, 2024 at 9:57 AM Masahiko Sawada  wrote:
>
> On Fri, Jun 14, 2024 at 9:41 AM Michael Paquier  wrote:
> >
> > On Thu, Jun 13, 2024 at 08:38:05PM -0400, Tom Lane wrote:
> > > Masahiko Sawada  writes:
> > >> I was about to push the patch but let me confirm just in case: is it
> > >> okay to bump the catversion even after post-beta1?
> > >
> > > Yes, that happens somewhat routinely.
> >
> > Up to RC, even after beta2.  This happens routinely every year because
> > tweaks are always required for what got committed.  And that's OK to
> > do so now.
>
> Thank you both for confirmation. I'll push it shortly.
>

Pushed. Thank you for giving feedback and reviewing the patch!

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: jsonpath: Missing Binary Execution Path?

[Chapman Flack]

On 06/13/24 16:43, David E. Wheeler wrote:
> Paging Mr. Eisentraut!

I'm not Mr. Eisentraut, but I have at last talked my way into some
access to the standard, so ...

Note 487 emphasizes that JSON path predicates "are not expressions;
instead they form a separate language that can only be invoked within
a ".

The only operators usable in a general expression (that is, a
 are binary + - and binary * / % and unary + -
over a .

Inside a filter, you get to use a . That's where
you can use ! and && and ||. But ! can only be applied to a
: either a ,
or any other  wrapped in parentheses.

On 06/13/24 11:32, David E. Wheeler wrote:
> david=# select jsonb_path_query('true', '$ && $');
> david=# select jsonb_path_query('true', '$.boolean() && $.boolean()');

Those don't work because, as you recognized, they're not inside filters.

> david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (@.boolean() && 
> @.boolean())');

That doesn't work because the operands of && or || must have the grammatical
form of predicates; it's not enough that they be expressions of boolean
type. '$[*] ? (@.boolean() == true && @.boolean() == true)' ought to work
(though in any other context you'd probably call it a code smell!) because
each operand is now a .

Regards,
-Chap

Re: race condition in pg_class

[Michael Paquier]

On Thu, Jun 13, 2024 at 05:35:49PM -0700, Noah Misch wrote:
> I think the attached covers all comments to date.  I gave everything v3, but
> most patches have just a no-conflict rebase vs. v2.  The exceptions are
> inplace031-inj-wait-event (implements the holding from that branch of the
> thread) and inplace050-tests-inj (updated to cooperate with inplace031).  Much
> of inplace031-inj-wait-event is essentially s/Extension/Custom/ for the
> infrastructure common to the two custom wait event types.

Looking at inplace031-inj-wait-event..

The comment at the top of GetWaitEventCustomNames() requires an
update, still mentioning extensions.

GetWaitEventCustomIdentifier() is incorrect, and should return
"InjectionPoint" in the default case of this class name, no?  I would
just pass the classID to GetWaitEventCustomIdentifier().

It is suboptimal to have pg_get_wait_events() do two scans of
WaitEventCustomHashByName.  Wouldn't it be better to do a single scan,
returning a set of (class_name,event_name) fed to the tuplestore of
this SRF?

 uint32
 WaitEventExtensionNew(const char *wait_event_name)
 {
+   return WaitEventCustomNew(PG_WAIT_EXTENSION, wait_event_name);
+}
+
+uint32
+WaitEventInjectionPointNew(const char *wait_event_name)
+{
+   return WaitEventCustomNew(PG_WAIT_INJECTIONPOINT, wait_event_name);
+}

Hmm.  The advantage of two routines is that it is possible to control
the class IDs allowed to use the custom wait events.  Shouldn't the
second routine be documented in xfunc.sgml?

wait_event_names.txt also needs tweaks, in the shape of a new class
name for the new class "InjectionPoint" so as it can be documented for
its default case.  That's a fallback if an event ID cannot be found,
which should not be the case, still that's more correct than showing
"Extension" for all class IDs covered by custom wait events.
--
Michael


signature.asc
Description: PGP signature

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Masahiko Sawada]

On Fri, Jun 14, 2024 at 9:41 AM Michael Paquier  wrote:
>
> On Thu, Jun 13, 2024 at 08:38:05PM -0400, Tom Lane wrote:
> > Masahiko Sawada  writes:
> >> I was about to push the patch but let me confirm just in case: is it
> >> okay to bump the catversion even after post-beta1?
> >
> > Yes, that happens somewhat routinely.
>
> Up to RC, even after beta2.  This happens routinely every year because
> tweaks are always required for what got committed.  And that's OK to
> do so now.

Thank you both for confirmation. I'll push it shortly.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Changing default -march landscape

[Thomas Munro]

On Thu, Jun 13, 2024 at 8:15 PM Magnus Hagander  wrote:
> Yeah, I think it's completely unreasonable  to push this on packagers and 
> just say "this is your problem now". If we do that, we can assume the only 
> people to get any benefit from these optimizations are those that use a fully 
> managed cloud service like azure or RDS.

It would also benefit distros that have decided to move their baseline
micro-arch level right now, probably without any additional action
from the maintainers assuming that gcc defaults to -march=*-v2 etc.
The cloud vendors' internal distros aren't really special in that
regard are they?

Hmm, among Linux distros, maybe it's really only Debian that isn't
moving or talking about moving the baseline yet?  (Are they?)

> They can do it, but we need to tell them how and when. And if we intend for 
> packagers to be part of the solution we need to explicitly bring them into 
> the discussion of how to do it, at a fairly early stage (and no, we can't 
> expect them to follow every thread on hackers).

OK let me CC Christoph and ask the question this way: hypothetically,
if RHEL users' PostgreSQL packages became automatically faster than
Debian users' packages because of default -march choice in the
toolchain, what would the Debian project think about that, and what
should we do about it?  The options discussed so far were:

1.  Consider a community apt repo that is identical to the one except
targeting the higher microarch levels, that users can point a machine
to if they want to.
2.  Various ideas for shipping multiple versions of the code at a
higher granularity than we're doing to day (a callback for a single
instruction!  the opposite extreme being the whole executable +
libraries), probably using some of techniques mentioned at
https://wiki.debian.org/InstructionSelection.

I would guess that 1 is about 100x easier but I haven't looked into it.

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Michael Paquier]

On Thu, Jun 13, 2024 at 08:38:05PM -0400, Tom Lane wrote:
> Masahiko Sawada  writes:
>> I was about to push the patch but let me confirm just in case: is it
>> okay to bump the catversion even after post-beta1?
> 
> Yes, that happens somewhat routinely.

Up to RC, even after beta2.  This happens routinely every year because
tweaks are always required for what got committed.  And that's OK to
do so now.
--
Michael


signature.asc
Description: PGP signature

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Tom Lane]

Masahiko Sawada  writes:
> I was about to push the patch but let me confirm just in case: is it
> okay to bump the catversion even after post-beta1?

Yes, that happens somewhat routinely.

regards, tom lane

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Masahiko Sawada]

On Tue, Jun 11, 2024 at 10:38 PM Masahiko Sawada  wrote:
>
> On Fri, Jun 7, 2024 at 10:22 AM Masahiko Sawada  wrote:
> >
> > On Wed, Jun 5, 2024 at 7:19 PM Andrey M. Borodin  
> > wrote:
> > >
> > >
> > >
> > > > On 4 Jun 2024, at 00:26, Masahiko Sawada  wrote:
> > >
> > > Thank you! Vacuum enhancement is a really good step forward, and this 
> > > small change would help a lot of observability tools.
> > >
> > >
> > > > On 4 Jun 2024, at 00:49, Peter Geoghegan  wrote:
> > > >
> > > > Can we rename this to num_dead_item_ids (or something similar) in
> > > > passing?
> > >
> > > I do not insist, but many tools will have to adapt to this change [0,1]. 
> > > However, most of tools will have to deal with removed max_dead_tuples 
> > > anyway [2], so this is not that big problem.
> >
> > True, this incompatibility would not be a big problem.
> >
> > num_dead_item_ids seems good to me. I've updated the patch that
> > incorporated the comment from Álvaro[1].
>
> I'm going to push the v2 patch in a few days if there is no further comment.
>

I was about to push the patch but let me confirm just in case: is it
okay to bump the catversion even after post-beta1? This patch
reintroduces a previously-used column to pg_stat_progress_vacuum so it
requires bumping the catversion.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: RFC: adding pytest as a supported test framework

[Jacob Champion]

On Thu, Jun 13, 2024 at 1:27 PM Robert Haas  wrote:
>
> On Thu, Jun 13, 2024 at 4:06 PM Jacob Champion
>  wrote:
> > There was a four-step plan sketch at the end of that email, titled "A
> > Plan". That was not intended to be "the final detailed plan", because
> > I was soliciting feedback on the exact pieces people wanted to try to
> > implement first, and I am not the God Emperor of Pytest. But it was
> > definitely A Plan.
>
> Well, OK, now I feel a bit dumb. I guess I missed that or forgot about it.

No worries. It's a really long thread. :D

But also: do you have opinions on what to fill in as steps 2
(something we have no ability to test today) and 3 (something we do
test today, but hate)?

My vote for step 2 is "client and server separation", perhaps by
testing libpq fallback against a server that claims support for
different build-time options. I don't want to have a vote in step 3,
because part of that step is proving that this framework can provide
value for a part of the project I don't really know much about.

> > I think this is way too much expectation for a v1 patch. If you were
> > committing this by yourself, would you agree to develop the entirety
> > of PostgreSQL::Test in a single commit, without the benefit of the
> > buildfarm checking you as you went, and other people trying to write
> > tests with it?
>
> Eh... I'm confused. PostgreSQL::Test::Cluster is more than half of the
> code in that directory, and without it you wouldn't be able to write
> most of the TAP tests that we have today.

Well, in my defense, you said "PostgreSQL::Test::whatever", which I
assumed meant all of it, including Kerberos.pm and SSL::Server and
AdjustUpgrade and... That seemed like way too much to me (and still
does!), but if that's not what you were arguing then never mind.

Yes, Cluster.pm seems like a pretty natural thing to ask for. I
imagine it's one of the first things we're going to need. And yet...

> You would really want to
> call this project done without having an equivalent?

...I have this really weird sneaking suspicion that, if a replacement
of the end-to-end Perl acceptance tests can be made an explicit
anti-goal in the short term, we might not necessarily need an
"equivalent" for v1. I realize that seems bizarre, because of course
we need a way to start the server if we want to test the server. But
frankly, starting a server is Pretty Easy (tm), and Cluster.pm has to
do a lot more than that because IMO it's designed for a variety of
acceptance-oriented tasks. 3000+ lines!

If there's widespread interest (as opposed to being just my own
personal fever dream) in testing Postgres components as individual
pieces rather than setting up the world, then I wonder if the
functionality from Cluster.pm couldn't be pared down a lot. Maybe you
don't need a centralized ->psql() or a ->command_ok() helper, because
you're usually not trying to test psql and other utilities during your
server-only tests.

Maybe you can just stand up a standby without a primary and drive it
via mock replication. Do you need quite as many "poll and wait for
some asynchronous result" type things when you're not waiting for a
result to cascade through a multinode system? Does something like (for
example) ->pg_recvlogical_upto() really have to be implemented in our
"core" fixtures or can it be done more easily by whoever needs that in
the future? Maybe You Ain't Gonna Need It.

If (he said, atop his naive idealistic soapbox) we can find a way to
put off writing utilities until we write the tests that need them,
without procrastinating, and without putting all of the negative
externalities of that approach on the committers with low-quality
copy-paste proliferation, and I'd like a pony while I'm at it, then I
think the result might end up being pretty coherent and maintainable.
Then not having "at least as much in-tree support for writing tests as
we have today" for the very first commit would be a feature and not a
bug.

Now, maybe if the collective ability to do that existed, we would have
done it already with Perl, but I do actually wonder whether that's
true or not.

Or, maybe, the very first suggestion for Step 3 will be something that
needs absolutely everything in Cluster.pm. So be it; I can live
without a pony.

> You would really want to
> call this project done without having an equivalent?

(A cop-out but not-really-cop-out alternative answer to this question
is that this project is not going to be "done" any more than Postgres
will ever be "done", and that's part of what I'm arguing should be
considered natural and okay. I understand that it is easier for me to
take that stance when I am not on the hook for maintaining it, so I
don't expect us to necessarily see eye-to-eye on it.)

> > Can you elaborate on why that's not an okay outcome?
>
> Well, you just argued that it should be an okay outcome, and I do sort
> of see your point, but I refer you to my earlier reply about the
> difficulty of getting anything 

Re: RFC: adding pytest as a supported test framework

[Jacob Champion]

On Thu, Jun 13, 2024 at 1:04 PM Robert Haas  wrote:
> One caveat here,
> perhaps, is that the focus of the work you've done up until now and
> the things that I and other community members may want as a condition
> of merging stuff may be somewhat distinct. You will have naturally
> been focused on your goals rather than other people's goals, or so I
> assume.

Right. That's a risk I knew I was taking when I wrote it, so it's not
going to offend me when I need to rewrite things.

> I would be a bit wary of splitting it up over
> too many different threads. It may well make sense to split it up, but
> it will probably be easier to review if the core work to enable this
> is one patch set on one thread where someone can read just that one
> thread and understand the situation, rather than many threads where
> you have to read them all.

I'll try to avoid too many threads. But right now there is indeed just
one thread (OAUTHBEARER) and it's way too much:

- the introduction of pytest
- a Construct-based manipulation of the wire protocol, including
Wireshark-style network traces on failure
- pytest fixtures which spin up libpq and the server in isolation from
each other, relying on the Construct implementation to complete the
seam
- OAuth, which was one of the motivating use cases (but not the only
one) for all of the previous items

I really don't want to derail this thread with those. There are other
people here with their own hopes and dreams (see: unconference notes),
and I want to give them a platform too.

> > That doesn't mean I want to
> > do this without a plan; it just means that the plan can involve saying
> > "this is not working and we can undo it" which makes the uncertainty
> > easier to take.
>
> As a community, we're really bad at this. [...]

I will carry the response to this to the next email.

Thanks,
--Jacob

Re: Logical Replication of sequences

[Michael Paquier]

On Thu, Jun 13, 2024 at 03:36:05PM +0530, Amit Kapila wrote:
> Fair enough. However, this raises the question Dilip and Vignesh are
> discussing whether we need a new relfilenode for sequence update even
> during initial sync? As per my understanding, the idea is that similar
> to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
> will create the new sequence entries in pg_subscription_rel with the
> state as 'i'. Then the sequence-sync worker would start a transaction
> and one-by-one copy the latest sequence values for each sequence (that
> has state as 'i' in pg_subscription_rel) and mark its state as ready
> 'r' and commit the transaction. Now if there is an error during this
> operation it will restart the entire operation.

Hmm.  You mean to use only one transaction for all the sequences?
I've heard about deployments with a lot of them.  Could it be a
problem to process them in batches, as well?  If you maintain a state
for each one of them in pg_subscription_rel, it does not strike me as
an issue, while being more flexible than an all-or-nothing.

> The idea of creating a
> new relfilenode is to handle the error so that if there is a rollback,
> the sequence state will be rolled back to 'i' and the sequence value
> will also be rolled back. The other option could be that we update the
> sequence value without a new relfilenode and if the transaction rolled
> back then only the sequence's state will be rolled back to 'i'. This
> would work with a minor inconsistency that sequence values will be
> up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> I am not sure if that matters because anyway, they can quickly be
> out-of-sync with the publisher again.

Seeing a mention to relfilenodes specifically for sequences freaks me
out a bit, because there's some work I have been doing in this area
and sequences may not have a need for a physical relfilenode at all.
But I guess that you refer to the fact that like tables, relfilenodes
would only be created as required because anything you'd do in the
apply worker path would just call some of the routines of sequence.h,
right?

> Now, say we don't want to maintain the state of sequences for initial
> sync at all then after the error how will we detect if there are any
> pending sequences to be synced? One possibility is that we maintain a
> subscription level flag 'subsequencesync' in 'pg_subscription' to
> indicate whether sequences need sync. This flag would indicate whether
> to sync all the sequences in pg_susbcription_rel. This would mean that
> if there is an error while syncing the sequences we will resync all
> the sequences again. This could be acceptable considering the chances
> of error during sequence sync are low.

There could be multiple subscriptions to a single database that point
to the same set of sequences.  Is there any conflict issue to worry
about here?

> The benefit is that both the
> REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> idea and sync all sequences without needing a new relfilenode. Users
> can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> all the sequences are synced after executing the command.

That would be cheaper, indeed.  Isn't a boolean too limiting?
Isn't that something you'd want to track with a LSN as "the point in
WAL where all the sequences have been synced"? 

The approach of doing all the sync work from the subscriber, while
having a command that can be kicked from the subscriber side is a good
user experience.
--
Michael


signature.asc
Description: PGP signature

Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY

[Michael Paquier]

On Tue, Jun 11, 2024 at 01:00:00PM +0200, Michail Nikolaev wrote:
> Probably something wrong with arbiter index selection for different
> backends. I am afraid it could be a symptom of a more serious issue.

ON CONFLICT selects an index that may be rebuilt in parallel of the
REINDEX happening, and its contents may be incomplete.  Isn't the
issue that we may select as arbiter indexes stuff that's !indisvalid?
Using the ccnew or ccold indexes would not be correct for the conflict
resolutions.
--
Michael


signature.asc
Description: PGP signature

Re: CI and test improvements

[Michael Paquier]

On Thu, Jun 13, 2024 at 06:56:20AM -0500, Justin Pryzby wrote:
> On Thu, Jun 13, 2024 at 02:38:46PM +0300, Nazir Bilal Yavuz wrote:
>>> I reintroduced the patch for ccache/windows -- v4.10 supports PCH, which
>>> can make the builds 2x faster.
>> 
>> I applied 0001 and 0002 to see ccache support on Windows but the build
>> step failed with: 'ccache: error: No stats log has been configured'.
>> Perhaps you forgot to add 'CCACHE_STATSLOG: $CCACHE_DIR.stats.log' to
>> 0002?
> 
> Something like that - I put the line back.  I don't know if statslog
> should be included in the patch, but it's useful for demonstrating that
> it's working.
> 
> ccache should be installed in the image rather than re-installed on each
> invocation.

Getting a 90s -> 30s improvement is nice.  With such numbers, 0002 is
worth considering first.

+ninja -C build |tee build.txt

In 0001, how OK is it to rely on the existence of tee for the VS2019
environments?  The base images include it, meaning that it is OK?

-REM choco install -y --no-progress ...

I'd rather keep this line in 0002, as a matter of documentation.

+set 
CC=c:\ProgramData\chocolatey\lib\ccache\tools\ccache-4.10-windows-x86_64\ccache.exe
 cl.exe

As of https://docs.mesa3d.org/meson.html#compiler-specification, using
CC is supported by meson (didn't know that), but shouldn't this be set
in the "env:" part of the VS2019 task in .cirrus.tasks.yml?
--
Michael


signature.asc
Description: PGP signature

Re: Shouldn't jsonpath .string() Unwrap?

[David E . Wheeler]

On Jun 13, 2024, at 3:53 PM, Andrew Dunstan  wrote:

> Hmm. You might be right. Many of these items have this code, but the string() 
> branch does not:
> if (unwrap && JsonbType(jb) == jbvArray)
>return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
>false);

Exactly, would be pretty easy to add. I can work up a patch this weekend.

D

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Thu, 13 Jun 2024 at 23:35, Andrew Dunstan  wrote:
> Clearly there are areas we can improve, and we need to be somewhat more
> proactive about it.

To follow that great suggestion, I updated meson wiki[1] after I
realized some of the major gripes I had with the Perl tap test output
were not actually caused by Perl but by meson:

The main changes I made was using "-q --print-errorlogs" instead "-v",
to reduce the enormous clutter in the output of the commands in the
wiki to something much more reasonable.

As well as adding examples on how to run specific tests

[1]: https://wiki.postgresql.org/wiki/Meson#Test_related_commands

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-13 Th 17:23, Robert Haas wrote:

On Thu, Jun 13, 2024 at 4:54 PM Andrew Dunstan  wrote:

FTR, I have put a lot of effort into maintaining and improving the
infrastructure over the years. And I don't think there is anything much
more important. So I'm going to put more effort in. And I'm not alone.
Andres, Alvaro, Noah and Thomas are some of those who have spent a lot
of effort on extending and improving our testing.

I appreciate the work you've done, and the work others have done, and
I'm sorry if my comments about the state of the project's
infrastructure came across as a personal attack. Some of what is wrong
here is completely outside of our control e.g. Perl is less popular.
And even there, some people have done heroic work, like Noah stepping
up to help maintain IPC::Run. And even with the stuff that is under
our control, it's not that I don't like what you're doing. It's rather
that I think we need more people doing it. For example, the fact that
nobody's helping Thomas maintain this cfbot that we all have come to
rely on, or helping him get that integrated into
commitfest.postgresql.org, is a problem. You're not on the hook to do
that, nor is anyone else. Likewise, the PostgreSQL::Test::whatever
modules are mostly evolving when it's absolutely necessary to get some
other patch committed, rather than anyone looking to improve them very
much for their own sake. Maybe part of the problem, as Greg said, is
that we don't do a good enough job advertising what the problems are
or how people can help, but whatever the cause, it's not a very
enjoyable experience, at least for me.

But again, I don't blame you for any of that. You're clearly a big
part of why it's going as well as it is!



Thank you, I'm not offended by anything you or anyone else has said. 
Clearly there are areas we can improve, and we need to be somewhat more 
proactive about it.



cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 4:54 PM Andrew Dunstan  wrote:
> FTR, I have put a lot of effort into maintaining and improving the
> infrastructure over the years. And I don't think there is anything much
> more important. So I'm going to put more effort in. And I'm not alone.
> Andres, Alvaro, Noah and Thomas are some of those who have spent a lot
> of effort on extending and improving our testing.

I appreciate the work you've done, and the work others have done, and
I'm sorry if my comments about the state of the project's
infrastructure came across as a personal attack. Some of what is wrong
here is completely outside of our control e.g. Perl is less popular.
And even there, some people have done heroic work, like Noah stepping
up to help maintain IPC::Run. And even with the stuff that is under
our control, it's not that I don't like what you're doing. It's rather
that I think we need more people doing it. For example, the fact that
nobody's helping Thomas maintain this cfbot that we all have come to
rely on, or helping him get that integrated into
commitfest.postgresql.org, is a problem. You're not on the hook to do
that, nor is anyone else. Likewise, the PostgreSQL::Test::whatever
modules are mostly evolving when it's absolutely necessary to get some
other patch committed, rather than anyone looking to improve them very
much for their own sake. Maybe part of the problem, as Greg said, is
that we don't do a good enough job advertising what the problems are
or how people can help, but whatever the cause, it's not a very
enjoyable experience, at least for me.

But again, I don't blame you for any of that. You're clearly a big
part of why it's going as well as it is!

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

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 2:52 PM Jelte Fennema-Nio  wrote:
> I understand and agree with your final stated goal of not ending up in
> another big mess. It's also clear to me that you don't think the
> current proposal achieves that goal. So I assume you have some
> additional ideas for the proposal to help achieve that goal and/or
> some specific worries that you'd like to get addressed better in the
> proposal. But currently it's not really clear to me what either of
> those are. Could you clarify?

Hmm, I don't know that I have what you're hoping I have, or at least
not any more than what I've said already.

I interpreted Jacob's original email as articulating a goal ("For the
v18 cycle, I would like to try to get pytest [1] in as a supported
test driver, in addition to the current offerings") rather than a
plan. There's no patch set yet and, as I understand it, no detailed
plan for a patch set: that email seemed to focus on the question of
desirability, rather than on outlining a plan of work, which I assume
is still to come. Some things I'd like to see when a patch set does
show up are:

- good documentation for people who have no previous experience with
Python and/or pytest e.g. here's how to set up your environment on
Linux, Windows, macOS, *BSD so you can run the tests, here's how to
run the tests, here's how it's different from the Perl framework we
have now

- no external dependencies on PostgreSQL connectors. psql or libpq
foreign function interface. the latter would be a cool increment of
progress over the status quo.

- at least as much in-tree support for writing tests as we have today
with PostgreSQL::Test::whatever, but not necessarily a 1:1 reinvention
of the stuff we have now, and documentation of those facilities that
is as good or, ideally, better than what we have today.

- high overall code quality and level of maturity, not just something
someone threw together for parity with the Perl system.

- enough tests written for or converted to the new system to give
reviewers confidence that it's truly usable and fit for purpose.

The important thing to me here (as it so often is) is to think like a
maintainer. Imagine that immediately after the patches for this
feature are committed, the developers who did the work all disappear
from the community and are never heard from again. How much pain does
that end us causing? The answer doesn't need to be zero; that is
unrealistic. But it also shouldn't be "well, if that happens we're
going to have to rip the feature out" or "well, a bunch of committers
who didn't want to write tests in Python in the first place are now
going to have to do a lot of work in Python to stabilize the work
already committed."

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

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 1:08 PM Jelte Fennema-Nio  wrote:
> I think 1 & 3 could be addressed by more easily giving commit/merge
> access to these tools than to the main PG repo. And I think 2 could be
> addressed by writing on the relevant wiki page where to go, and
> probably putting a link to the wiki page on the actual website of the
> tool.

+1.

> But Perl is at the next level of unmaintained infrastructure. It is
> actually clear how you can contribute to it, but still no new
> community members actually want to contribute to it. Also, it's not
> only unmaintained by us but it's also pretty much unmaintained by the
> upstream community.

I feel like I already agreed to this in a previous email and you're
continuing to argue with me as if I were disagreeing.

> As you said, no one in our community wants to maintain our testsuite
> full time. But our test suite consists partially of upstream
> dependencies and partially of our own code. Right now pretty much
> no-one improves the ustream code, and pretty much no-one improves our
> own code. Using a more modern language gives up much more frequent
> upstream improvements for free, and it will allow new community
> members to contribute to our own test suite.

I also agree with this. I'm just not super optimistic about how much
of that will actually happen. And I'd like to hear you acknowledge
that concern and think about whether it can be addressed in some way,
instead of just repeating that we should do it anyway. Because I agree
we probably should do it anyway, but that doesn't mean I wouldn't like
to see the downsides mitigated as much as we can. In particular, if
the proposal is exactly "let's add the smallest possible patch that
enables people to write tests in Python and then add a few new tests
in Python while leaving almost everything else in Perl, with no
migration plan and no clear vision of how the Python support ever gets
any better than the minimum stub that is proposed for initial commit,"
then I don't know that I can vote for that plan. Honestly, that sounds
like very little work for the person proposing that minimal patch and
a whole lot of work for the rest of the community later on, and the
evidence is not in favor of volunteers showing up to take care of that
work. The plan should be more front-loaded than that: enough initial
development should get done by the people making the proposal that if
the work stops after, we don't have another big mess on our hands.

Or so I think, anyway.

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

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Thu, 13 Jun 2024 at 20:11, Robert Haas  wrote:
> > But Perl is at the next level of unmaintained infrastructure. It is
> > actually clear how you can contribute to it, but still no new
> > community members actually want to contribute to it. Also, it's not
> > only unmaintained by us but it's also pretty much unmaintained by the
> > upstream community.
>
> I feel like I already agreed to this in a previous email and you're
> continuing to argue with me as if I were disagreeing.

Sorry about that.

> I also agree with this. I'm just not super optimistic about how much
> of that will actually happen. And I'd like to hear you acknowledge
> that concern and think about whether it can be addressed in some way,
> instead of just repeating that we should do it anyway. Because I agree
> we probably should do it anyway, but that doesn't mean I wouldn't like
> to see the downsides mitigated as much as we can.

I'm significantly more optimistic than you, but I also definitely
understand and agree with the concern. I also agree that mitigating
that concern beforehand would be a good thing.

> In particular, if
> the proposal is exactly "let's add the smallest possible patch that
> enables people to write tests in Python and then add a few new tests
> in Python while leaving almost everything else in Perl, with no
> migration plan and no clear vision of how the Python support ever gets
> any better than the minimum stub that is proposed for initial commit,"
> then I don't know that I can vote for that plan. Honestly, that sounds
> like very little work for the person proposing that minimal patch and
> a whole lot of work for the rest of the community later on, and the
> evidence is not in favor of volunteers showing up to take care of that
> work. The plan should be more front-loaded than that: enough initial
> development should get done by the people making the proposal that if
> the work stops after, we don't have another big mess on our hands.
>
> Or so I think, anyway.

I understand and agree with your final stated goal of not ending up in
another big mess. It's also clear to me that you don't think the
current proposal achieves that goal. So I assume you have some
additional ideas for the proposal to help achieve that goal and/or
some specific worries that you'd like to get addressed better in the
proposal. But currently it's not really clear to me what either of
those are. Could you clarify?

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 3:17 PM Greg Sabino Mullane  wrote:
> I feel at least some of this is a visibility / marketing problem. I've not 
> seen any dire requests for help come across on the lists, nor things on the 
> various todos/road maps/ blog posts people make from time to time. If I had, 
> I would have jumped in. And for the record, I'm very proficient with Perl.

I agree with all of that!

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

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-12 We 18:43, Jelte Fennema-Nio wrote:


I agree it's not a technical issue. It is a people issue. There are
very few people skilled in Perl active in the community. And most of
those are very senior hackers that have much more important things to
do that make our Perl testing framework significantly better. And the
less senior people that might see improving tooling as a way to get
help out in the community, are try to stay away from Perl with a 10
foot pole. So the result is, nothing gets improved. Especially since
very few people outside our community improve this tooling either.

  



FTR, I have put a lot of effort into maintaining and improving the 
infrastructure over the years. And I don't think there is anything much 
more important. So I'm going to put more effort in. And I'm not alone. 
Andres, Alvaro, Noah and Thomas are some of those who have spent a lot 
of effort on extending and improving our testing.


People tend to get a bit hung up about languages. I lost count of the 
various languages I had learned when it got somewhere north of 30.


Still, I understand that perl has a few oddities that make people 
scratch their heads (as do most languages). It's probably losing market 
share, along with some of the other things we rely on. Not sure that 
alone is a reason to move away from it.



cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Jacob Champion]

On Thu, Jun 13, 2024 at 11:11 AM Robert Haas  wrote:
> I feel like I already agreed to this in a previous email and you're
> continuing to argue with me as if I were disagreeing.

I also think that maybe arguments are starting to sail past each
other, and the temperature is starting to climb. (And Jelte may be
arguing to all readers of the thread, rather than just a single
individual. It's hard to tell with the list format.) And now I see
that there's another email that came in while I was writing this, but
I think I'm going to have to send this as-is because I can't write
emails that fast.

> I also agree with this. I'm just not super optimistic about how much
> of that will actually happen. And I'd like to hear you acknowledge
> that concern and think about whether it can be addressed in some way,
> instead of just repeating that we should do it anyway. Because I agree
> we probably should do it anyway, but that doesn't mean I wouldn't like
> to see the downsides mitigated as much as we can.

Okay, +1.

> In particular, if
> the proposal is exactly "let's add the smallest possible patch that
> enables people to write tests in Python and then add a few new tests
> in Python while leaving almost everything else in Perl, with no
> migration plan and no clear vision of how the Python support ever gets
> any better than the minimum stub that is proposed for initial commit,"
> then I don't know that I can vote for that plan.

(that's not the proposal and I know/think you know that but having my
original email twisted into that is making me feel a bit crispy)

I do not want to migrate, and I have stated so multiple times, which
is why I have not proposed a migration plan. Other committers have
already expressed resistance to the idea that we would rewrite the
Perl stuff. I think they're right. I think we should not. I think we
should accept the cost of both Perl and something else for the
near-to-medium future, as long as the "something else" gives us value
offsetting the additional cost.

> Honestly, that sounds
> like very little work for the person proposing that minimal patch and
> a whole lot of work for the rest of the community later on, and the
> evidence is not in favor of volunteers showing up to take care of that
> work.

Okay, cool. Here: as the person who is 100% signing himself up to do
that, time for me to jump in.

I have an entire 6000-something-line suite of protocol tests that has
been linked like four times above. It does something fundamentally
different from the end-to-end Perl suite; it is not a port. It is far
from perfect and I do not want to pressure people to adopt it as-is,
which is why I have not. In this thread, I am offering it solely as
evidence that I have follow-up intent.

But I might get hit by a bus. Or, as far as anyone except me knows, I
might lose interest after things get hard, which would be sad. Which
is why my very first proposal was to add an entry point that can be
reverted. The suite is not going to infect the codebase any more than
the Perl does. A revert will involve pulling the Meson test entry
code, and deleting all pytest subdirectories (of which there is only
one, IIRC, in my OAuth suite).

> The plan should be more front-loaded than that: enough initial
> development should get done by the people making the proposal that if
> the work stops after, we don't have another big mess on our hands.

For me personally, the problem is the opposite. I have done _so much_
initial development by myself that there's no way it could ever be
reviewed and accepted. But I had to do that to get meaningful
development done in my style of work, which is focused on security and
testability and verifiable implementation.

I am trying to carve off pieces of that and say "hey, does this look
nice to anyone else?" That will take time, probably over multiple
different threads. In the meantime, I don't want to be a serialization
point for other people who are excited about trying new testing
methods, because very few people are currently doing the exact kind of
work I am doing. They may want to do other things, as evidenced by the
thread contents. At least one committer would have to sign up to be a
serialization point, unfortunately, but I think that's going to be
true regardless of plan, if we want multiple non-committer members of
the community to be involved instead of just one torch-bearer.

Because of how many moving parts and competing interests and personal
disagreements there are, I am firmly in the camp of "try something
that many people think *might* work better, that can be undone if it
sucks, and iterate on it." I want to build community momentum, because
I think that's a pretty effective way to change the cultural norms
that you're saying you're frustrated with. That doesn't mean I want to
do this without a plan; it just means that the plan can involve saying
"this is not working and we can undo it" which makes the uncertainty
easier to take.

--Jacob

Re: jsonpath: Missing Binary Execution Path?

[Andrew Dunstan]

On 2024-06-13 Th 11:37, David E. Wheeler wrote:

On Jun 13, 2024, at 11:32, David E. Wheeler  wrote:


Should && and || not also work on scalar operands?

I see the same issue for unary !, too:



What does the spec say about these? What do other implementations do?


cheers


andrew

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

Re: jsonpath: Missing Binary Execution Path?

[David E. Wheeler]

On Jun 13, 2024, at 3:33 PM, Andrew Dunstan  wrote:

> What does the spec say about these? What do other implementations do?

Paging Mr. Eisentraut!

:-)

D

Re: RFC: adding pytest as a supported test framework

[Greg Sabino Mullane]

On Thu, Jun 13, 2024 at 9:38 AM Robert Haas  wrote:

> I agree with you, but I'm skeptical that solving it will be as easy as
> switching to Python. For whatever reason, it seems like every piece of
> infrastructure that the PostgreSQL community has suffers from severe
> neglect. Literally everything I know of either has one or maybe two
> very senior hackers maintaining it, or no maintainer at all.

...

> All of this stuff is critical project infrastructure and yet it feels like
> nobody wants to work on
> it.


I feel at least some of this is a visibility / marketing problem. I've not
seen any dire requests for help come across on the lists, nor things on the
various todos/road maps/ blog posts people make from time to time. If I
had, I would have jumped in. And for the record, I'm very proficient with
Perl.

Cheers,
Greg

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-13 Th 15:16, Greg Sabino Mullane wrote:

I'm very proficient with Perl.




Yes you are, and just as well!


cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-12 We 11:50, Andres Freund wrote:

Hi,

On 2024-06-11 07:28:23 -0700, Jacob Champion wrote:

On Mon, Jun 10, 2024 at 1:04 PM Andres Freund  wrote:

Just for context for the rest the email: I think we desperately need to move
off perl for tests. The infrastructure around our testing is basically
unmaintained and just about nobody that started doing dev stuff in the last 10
years learned perl.

Okay. Personally, I'm going to try to stay out of discussions around
subtracting Perl and focus on adding Python, for a bunch of different
reasons:

I think I might have formulated my paragraph above badly - I didn't mean that
we should move away from perl tests tomorrow,



OK, glad we're on the same page there. Let's move on.



but that we need a path forward
that allows folks to write tests without perl.



OK, although to be honest I'm more interested in fixing some of the 
things that have made testing with perl a pain, especially the IPC::Run 
pump stuff.



cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 4:06 PM Jacob Champion
 wrote:
> There was a four-step plan sketch at the end of that email, titled "A
> Plan". That was not intended to be "the final detailed plan", because
> I was soliciting feedback on the exact pieces people wanted to try to
> implement first, and I am not the God Emperor of Pytest. But it was
> definitely A Plan.

Well, OK, now I feel a bit dumb. I guess I missed that or forgot about it.

> > - at least as much in-tree support for writing tests as we have today
> > with PostgreSQL::Test::whatever, but not necessarily a 1:1 reinvention
> > of the stuff we have now, and documentation of those facilities that
> > is as good or, ideally, better than what we have today.
>
> I think this is way too much expectation for a v1 patch. If you were
> committing this by yourself, would you agree to develop the entirety
> of PostgreSQL::Test in a single commit, without the benefit of the
> buildfarm checking you as you went, and other people trying to write
> tests with it?

Eh... I'm confused. PostgreSQL::Test::Cluster is more than half of the
code in that directory, and without it you wouldn't be able to write
most of the TAP tests that we have today. You would really want to
call this project done without having an equivalent?

> > The important thing to me here (as it so often is) is to think like a
> > maintainer. Imagine that immediately after the patches for this
> > feature are committed, the developers who did the work all disappear
> > from the community and are never heard from again. How much pain does
> > that end us causing? The answer doesn't need to be zero; that is
> > unrealistic. But it also shouldn't be "well, if that happens we're
> > going to have to rip the feature out"
>
> Can you elaborate on why that's not an okay outcome?

Well, you just argued that it should be an okay outcome, and I do sort
of see your point, but I refer you to my earlier reply about the
difficulty of getting anything reverted in the culture as it stands.

> > or "well, a bunch of committers
> > who didn't want to write tests in Python in the first place are now
> > going to have to do a lot of work in Python to stabilize the work
> > already committed."
>
> Is it that? If the problem is that, we should address that. Because if
> that is truly the fear, I cannot assuage that fear without showing you
> something, and I cannot show you something you do not want to see, if
> you don't want to write tests in Python in the first place.

I have zero desire to write tests in Python. If I could convince
everyone here to spend their time and energy improving the stuff we
have in Perl instead of introducing a whole new test framework, I
would 100% do that. But I'm pretty sure that I can't, and I think the
project needs to pick from among realistic options rather than
theoretical ones. Said differently, it's not all about me.

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

Re: RFC: adding pytest as a supported test framework

[Daniel Gustafsson]

> On 13 Jun 2024, at 20:09, Melanie Plageman  wrote:
> 
> On Thu, Jun 13, 2024 at 7:27 AM Daniel Gustafsson  wrote:
>> 
>>> On 13 Jun 2024, at 00:34, Melanie Plageman  
>>> wrote:
>> 
>>> FWIW, I felt a lot of pain trying to write recovery TAP tests with
>>> IPC::Run's pumping functionality. It was especially painful (as
>>> someone who knows even less Perl than the "street fighting Perl"
>>> Thomas Munro has described having) before the additional test
>>> infrastructure was added in BackgroudPsql.pm last year.
>> 
>> A key aspect of this, which isn't specific to Perl or our use of it, is that
>> this was done in backbranches which doesn't have the (recently) much improved
>> BackgroundPsql.pm.  The quality of our tools and the ease of use they provide
>> is directly related to the investment we make into continuously improving our
>> testharness.  Regardless of which toolset we adopt, if we don't make this
>> investment (taking learnings from the past years and toolsets into account)
>> we're bound to repeat this thread in a few years advocating for toolset X+1.
> 
> True. And thank you for committing BackgroundPsql.pm (and Andres for
> starting that work). My specific case is likely one of a poor work
> person blaming her tools :)

I don't think it is since the tools we had then were really hard to use.  I
wrote very similar tests to yours for the online checksums patch and they were
quite complicated to get right.  The point is that the complexity was greatly
reduced by the community, and that kind of work will be equally important
regardless of toolset.

--
Daniel Gustafsson

Re: race condition in pg_class

[Noah Misch]

On Wed, Jun 12, 2024 at 10:02:00PM +0200, Michail Nikolaev wrote:
> > Can you say more about the connection you see between $SUBJECT and that?
> That
> > looks like a valid report of an important bug, but I'm not following the
> > potential relationship to $SUBJECT.
> 
> I was guided by the following logic:
> * A pg_class race condition can cause table indexes to look stale.
> * REINDEX updates indexes
> * errors can be explained by different backends using different arbiter
> indexes

Got it.  The race condition of $SUBJECT involves inplace updates, and the
wrong content becomes permanent.  Hence, I suspect they're unrelated.

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-12 We 18:34, Melanie Plageman wrote:

On Tue, Jun 11, 2024 at 8:05 AM Andrew Dunstan  wrote:


On 2024-06-10 Mo 21:49, Andres Freund wrote:

Hi,

On 2024-06-10 16:46:56 -0400, Andrew Dunstan wrote:

I'm not sure what part of the testing infrastructure you think is
unmaintained. For example, the last release of Test::Simple was all the way
back on April 25.

IPC::Run is quite buggy and basically just maintained by Noah these days.


Yes, that's true. I think the biggest pain point is possibly the recovery tests.

Some time ago I did some work on wrapping libpq using the perl FFI module. It 
worked pretty well, and would mean we could probably avoid many uses of 
IPC::Run, and would probably be substantially more efficient (no fork 
required). It wouldn't avoid all uses of IPC::Run, though.

But my point was mainly that while a new framework might have value, I don't 
think we need to run out and immediately rewrite several hundred TAP tests. 
Let's pick the major pain points and address those.

FWIW, I felt a lot of pain trying to write recovery TAP tests with
IPC::Run's pumping functionality. It was especially painful (as
someone who knows even less Perl than the "street fighting Perl"
Thomas Munro has described having) before the additional test
infrastructure was added in BackgroudPsql.pm last year. As an example
of the "worst case", it took me two full work days to go from a repro
(with psql sessions on a primary and replica node) of the vacuum hang
issue being explored in [1] to a sort-of working TAP test which
demonstrated it - and that was with help from several other
committers. Granted, this is a complex case.



The pump stuff is probably the least comprehensible and most fragile 
part of the whole infrastructure. As I just mentioned to Andres, I'm 
hoping to make a substantial improvement in that area.





A small part of the issue is that, as Tristan has said elsewhere,
there aren't good developer tool integrations that I know about for
Perl. I use neovim's LSP support for C and Python (in other projects),
and there is a whole ecosystem of tools I can use for both C and
Python. I know not everyone likes or needs these, but I find that they
help me write and debug code faster.



You might find this useful: 



(I don't use neovim - I'm an old emacs dinosaur.)




I had offered to take a stab at writing some of the BackgroundPsql
test infrastructure in Python. I haven't started exploring that yet or
looking at what Jacob has done so far, but I am optimistic that this
is an area where it is worth seeing what is available to us outside of
IPC::Run.



Yeah, like I said, I'm working on reducing our reliance on especially 
the more fragile parts of IPC::Run.



cheers


andrew

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

Re: improve predefined roles documentation

[Nathan Bossart]

On Thu, Jun 13, 2024 at 01:05:33PM -0700, David G. Johnston wrote:
> One of the main attributes for the GUCs is their category.  If we want to
> improve organization we'd need to assign categories first.  We already
> implicitly do so in the description section where we do group them together
> and explain why - but it is all informal.  But getting rid of those
> groupings and descriptions and isolating each role so it can be linked to
> more easily seems like a net loss in usability.

What I had in mind is that we would retain these groupings.  I agree that
isolating roles like pg_read_all_data and pg_write_all_data would be no
good.

-- 
nathan

Re: RFC: adding pytest as a supported test framework

[Jacob Champion]

On Thu, Jun 13, 2024 at 12:20 PM Robert Haas  wrote:
> I interpreted Jacob's original email as articulating a goal ("For the
> v18 cycle, I would like to try to get pytest [1] in as a supported
> test driver, in addition to the current offerings") rather than a
> plan.

That's the first part of it.

> There's no patch set yet and, as I understand it, no detailed
> plan for a patch set: that email seemed to focus on the question of
> desirability, rather than on outlining a plan of work, which I assume
> is still to come.

There was a four-step plan sketch at the end of that email, titled "A
Plan". That was not intended to be "the final detailed plan", because
I was soliciting feedback on the exact pieces people wanted to try to
implement first, and I am not the God Emperor of Pytest. But it was
definitely A Plan.

> Some things I'd like to see when a patch set does
> show up are:
>
> - good documentation for people who have no previous experience with
> Python and/or pytest e.g. here's how to set up your environment on
> Linux, Windows, macOS, *BSD so you can run the tests, here's how to
> run the tests, here's how it's different from the Perl framework we
> have now

+1

> - no external dependencies on PostgreSQL connectors. psql or libpq
> foreign function interface. the latter would be a cool increment of
> progress over the status quo.

If this is a -1 for psycopg, then I will cast my vote for ctypes/CFFI
and against psql.

> - at least as much in-tree support for writing tests as we have today
> with PostgreSQL::Test::whatever, but not necessarily a 1:1 reinvention
> of the stuff we have now, and documentation of those facilities that
> is as good or, ideally, better than what we have today.

I think this is way too much expectation for a v1 patch. If you were
committing this by yourself, would you agree to develop the entirety
of PostgreSQL::Test in a single commit, without the benefit of the
buildfarm checking you as you went, and other people trying to write
tests with it?

> - high overall code quality and level of maturity, not just something
> someone threw together for parity with the Perl system.

+1

> - enough tests written for or converted to the new system to give
> reviewers confidence that it's truly usable and fit for purpose.

This is that "know everything up front" tax that I think is not
reasonable for a test framework. If the thing you're trying to avoid
is the foot-in-the-door phenomenon, I would agree with you for a
Postgres feature. But these are tests; we don't ship them, we have
different rules for backporting them, they are developed in a very
different way.

> The important thing to me here (as it so often is) is to think like a
> maintainer. Imagine that immediately after the patches for this
> feature are committed, the developers who did the work all disappear
> from the community and are never heard from again. How much pain does
> that end us causing? The answer doesn't need to be zero; that is
> unrealistic. But it also shouldn't be "well, if that happens we're
> going to have to rip the feature out"

Can you elaborate on why that's not an okay outcome?

> or "well, a bunch of committers
> who didn't want to write tests in Python in the first place are now
> going to have to do a lot of work in Python to stabilize the work
> already committed."

Is it that? If the problem is that, we should address that. Because if
that is truly the fear, I cannot assuage that fear without showing you
something, and I cannot show you something you do not want to see, if
you don't want to write tests in Python in the first place.

--Jacob

Re: improve predefined roles documentation

[David G. Johnston]

On Thu, Jun 13, 2024 at 12:48 PM Nathan Bossart 
wrote:

> I think we could improve matters by abandoning the table and instead
> documenting these roles more like we document GUCs, i.e., each one has a
> section below it where we can document it in as much detail as we want.
>
>
One of the main attributes for the GUCs is their category.  If we want to
improve organization we'd need to assign categories first.  We already
implicitly do so in the description section where we do group them together
and explain why - but it is all informal.  But getting rid of those
groupings and descriptions and isolating each role so it can be linked to
more easily seems like a net loss in usability.

I'm against getting rid of the table.  If we do add authoritative
subsection anchors we should just do like we do in System Catalogs and make
the existing table name values hyperlinks to those newly added anchors.
Breaking the one table up into multiple tables along category lines is
something to consider.

David J.

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-12 We 11:28, Andres Freund wrote:

Hi,

On 2024-06-11 08:04:57 -0400, Andrew Dunstan wrote:

Some time ago I did some work on wrapping libpq using the perl FFI module.
It worked pretty well, and would mean we could probably avoid many uses of
IPC::Run, and would probably be substantially more efficient (no fork
required). It wouldn't avoid all uses of IPC::Run, though.

FWIW, I'd *love* to see work on this continue. The reduction in test runtime
on windows is substantial and would shorten the hack->CI->fail->hack loop a
good bit shorter. And save money.



OK, I will put it high on my list. I just did some checking and it seems 
to be feasible on Windows. StrawberryPerl at least has FFI::Platypus out 
of the box, so we would not need to turn any great handsprings to make 
progress on this on a fairly wide variety of platforms.


What seems a good place to start would be a simple 
PostgreSQL::Test::Session object that would allow us to get rid of a 
whole heap of start/pump_until/kill cycles and deal with the backend in 
a much more straightforward and comprehensible way, not to mention the 
possibility of removing lots of $node->{safe_}psql calls.



cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 3:28 PM Jacob Champion
 wrote:
> (that's not the proposal and I know/think you know that but having my
> original email twisted into that is making me feel a bit crispy)

I definitely did not mean to imply that. I took your original email as
a goal, rather than a proposal or plan. My statement was strictly
intended as a hypothetical because I didn't think any plan had been
proposed - I only meant to say that *if* the plan were to do X, that
would be a hard sell for me.

> I do not want to migrate, and I have stated so multiple times, which
> is why I have not proposed a migration plan. Other committers have
> already expressed resistance to the idea that we would rewrite the
> Perl stuff. I think they're right. I think we should not. I think we
> should accept the cost of both Perl and something else for the
> near-to-medium future, as long as the "something else" gives us value
> offsetting the additional cost.

I agree. It's not terribly pretty, IMHO, but it's hard to see doing
things any other way.

> For me personally, the problem is the opposite. I have done _so much_
> initial development by myself that there's no way it could ever be
> reviewed and accepted. But I had to do that to get meaningful
> development done in my style of work, which is focused on security and
> testability and verifiable implementation.

I admire this attitude. I think a lot of people who go off and do a
ton of initial work outside core show up and are like "ok, now take
all of my code." As you say, that's not realistic. One caveat here,
perhaps, is that the focus of the work you've done up until now and
the things that I and other community members may want as a condition
of merging stuff may be somewhat distinct. You will have naturally
been focused on your goals rather than other people's goals, or so I
assume.

> I am trying to carve off pieces of that and say "hey, does this look
> nice to anyone else?" That will take time, probably over multiple
> different threads.

This makes sense, but I would be a bit wary of splitting it up over
too many different threads. It may well make sense to split it up, but
it will probably be easier to review if the core work to enable this
is one patch set on one thread where someone can read just that one
thread and understand the situation, rather than many threads where
you have to read them all.

> Because of how many moving parts and competing interests and personal
> disagreements there are, I am firmly in the camp of "try something
> that many people think *might* work better, that can be undone if it
> sucks, and iterate on it." I want to build community momentum, because
> I think that's a pretty effective way to change the cultural norms
> that you're saying you're frustrated with. That doesn't mean I want to
> do this without a plan; it just means that the plan can involve saying
> "this is not working and we can undo it" which makes the uncertainty
> easier to take.

As a community, we're really bad at this. Once something gets
committed, getting a consensus to revert it is really hard, especially
if a major release has happened meanwhile, but most of the time even
if it hasn't. It might be a little easier in this case, since after
all it's not a directly user-visible feature. But historically what
happens if somebody says "hey, there are six unfixed problems with
this feature!" is that everybody says "well, you're free to fix the
problems if you want, but you're not allowed to revert the feature."
And that is *exactly* how we end up with stuff like the current TAP
test framework: ripping that out would mean removing all the TAP tests
that depend on it, and that wouldn't have achieved consensus two
months after the feature went in, let alone today.

Now, it has been suggested to me by at least one other person involved
with the project that we need to be more open to the kind of thing
that you propose here: add experimental things and take them out if it
doesn't work out. I can definitely understand that this might be a
culturally better approach than what we currently do. So maybe that's
the way forward, but it is hard (at least for me) to get past the fear
of being the one left holding the bag, and I suspect that other
committers have similar fears. What exactly we should do about that,
I'm not sure.

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

Re: Shouldn't jsonpath .string() Unwrap?

[Andrew Dunstan]

On 2024-06-12 We 16:02, David G. Johnston wrote:
On Sat, Jun 8, 2024 at 3:50 PM David E. Wheeler 
 wrote:


Hackers,

Most of the jsonpath methods auto-unwrap in lax mode:

david=# select jsonb_path_query('[-2,5]', '$.abs()');
 jsonb_path_query
--
 2
 5
(2 rows)

The obvious exceptions are size() and type(), which apply directly
to arrays, so no need to unwrap:

david=# select jsonb_path_query('[-2,5]', '$.size()');
 jsonb_path_query
--
 2
(1 row)

david=# select jsonb_path_query('[-2,5]', '$.type()');
 jsonb_path_query
--
 "array"

But what about string()? Is there some reason it doesn’t unwrap?

david=# select jsonb_path_query('[-2,5]', '$.string()');
ERROR:  jsonpath item method .string() can only be applied to a
bool, string, numeric, or datetime value

What I expect:

david=# select jsonb_path_query('[-2,5]', '$.string()');
 jsonb_path_query
—
 "2"
 "5"
(2 rows)

However, I do see a test[1] for this behavior, so maybe there’s a
reason for it?


Adding Andrew.

I'm willing to call this an open item against this feature as I don't 
see any documentation explaining that string() behaves differently 
than the others.





Hmm. You might be right. Many of these items have this code, but the 
string() branch does not:


   if (unwrap && JsonbType(jb) == jbvArray)
    return executeItemUnwrapTargetArray(cxt, jsp, jb, found,
    false);


cheers


andrew

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

improve predefined roles documentation

[Nathan Bossart]

IMHO there are a couple of opportunities for improving the predefined roles
documentation [0]:

* Several of the roles in the table do not have corresponding descriptions
  in the paragraphs below the table (e.g., pg_read_all_data,
  pg_write_all_data, pg_checkpoint, pg_maintain,
  pg_use_reserved_connections, and pg_create_subscription).  Furthermore,
  IMHO it is weird to have some of the information in the table and some
  more in a paragraph down the page.

* The table has grown quite a bit over the years, but the entries are
  basically unordered, requiring readers to perform a linear search (O(n))
  to find information about a specific role.

* Documentation that refers to these roles cannot link to a specific one.
  Currently, we just link to the page or the table.

I think we could improve matters by abandoning the table and instead
documenting these roles more like we document GUCs, i.e., each one has a
section below it where we can document it in as much detail as we want.
Some of these roles should probably be documented together (e.g.,
pg_read_all_data and pg_write_all_data), so the ordering is unlikely to be
perfect, but I'm hoping it would still be a net improvement.

Thoughts?

[0] https://www.postgresql.org/docs/devel/predefined-roles.html

-- 
nathan

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Thu, 13 Jun 2024 at 15:38, Robert Haas  wrote:
> For whatever reason, it seems like every piece of
> infrastructure that the PostgreSQL community has suffers from severe
> neglect. Literally everything I know of either has one or maybe two
> very senior hackers maintaining it, or no maintainer at all. Andrew
> maintains the buildfarm and it evolves quite slowly. Andres did all
> the work on meson, with some help from Peter. Thomas maintains cfbot
> as a skunkworks. The Perl-based TAP test framework gets barely any
> love at all. The CommitFest application is pretty much totally
> stagnant, and in fact is a great example of what I'm talking about
> here: I wrote an original version in Perl and somebody -- I think
> Magnus -- rewrote it in a more maintainable framework -- and then the
> development pace went to basically zero. All of this stuff is critical
> project infrastructure and yet it feels like nobody wants to work on
> it.

Overall, I agree with the sentiment of us not maintaining our tooling
well (although I think meson maintenance has been pretty decent so
far). I think there's a bunch of reasons for this (not all apply to
each of the tools):
1. pretty much solely maintained by senior community members who don't
have time to maintain it
2. no clear way to contribute. e.g. where should I send a patch/PR for
the commitfest app, or the cfbot?
3. (related to 1) unresponsive when somehow contributions are actually
sent in (I have two open PRs on the cfbot app from 3 years ago without
any response)

I think 1 & 3 could be addressed by more easily giving commit/merge
access to these tools than to the main PG repo. And I think 2 could be
addressed by writing on the relevant wiki page where to go, and
probably putting a link to the wiki page on the actual website of the
tool.

But Perl is at the next level of unmaintained infrastructure. It is
actually clear how you can contribute to it, but still no new
community members actually want to contribute to it. Also, it's not
only unmaintained by us but it's also pretty much unmaintained by the
upstream community.

> But I still
> think it's fair to question whether the preference of many developers
> for Python over Perl will translate into sustained investment in
> improving the infrastructure. Again, I will be thrilled if it does,
> but that just doesn't seem to be the way that things go around here,
> and I bet the reasons go well beyond choice of programming language.

As you said, no one in our community wants to maintain our testsuite
full time. But our test suite consists partially of upstream
dependencies and partially of our own code. Right now pretty much
no-one improves the ustream code, and pretty much no-one improves our
own code. Using a more modern language gives up much more frequent
upstream improvements for free, and it will allow new community
members to contribute to our own test suite.

And I understand you are sceptical that people will contribute to our
own test suite, just because it's Python. But as a counterpoint:
people are currently already doing exactly that, just outside of the
core postgres repo[1][2][3]. I don't see why those people would
suddenly stop doing that if we include such a suite in the official
repo. Apparently many people hate writing tests in Perl so much that
they'd rather build Python test frameworks to test their extensions,
than to use/improve the Perl testing framework included in Postgres.

[1]: https://github.com/pgbouncer/pgbouncer/tree/master/test
[2]: https://github.com/jchampio/pg-pytest-suite
[3]: https://github.com/postgrespro/testgres


PS. I don't think it makes sense to host our tooling like the
commitfest app on our own git server instead of github/gitlab. That
only makes it harder for community members to contribute and also much
harder to set up CI. I understand the reasons why we use mailing lists
for the development of core postgres, but I don't think those apply
nearly as much to our tooling repos. And honestly also not to stuff
like the website.

Re: Avoid orphaned objects dependencies, take 3

[Bertrand Drouvot]

Hi,

On Thu, Jun 13, 2024 at 10:49:34AM -0400, Robert Haas wrote:
> On Fri, Jun 7, 2024 at 4:41 AM Bertrand Drouvot
>  wrote:
> > Do you still find the code hard to maintain with v9?
> 
> I don't think it substantially changes my concerns as compared with
> the earlier version.

Thanks for the feedback, I'll give it more thoughts.

> 
> > > but we're not similarly careful about other operations e.g.
> > > ConstraintSetParentConstraint is called by DefineIndex which calls
> > > table_open(childRelId, ...) first, but there's no logic in DefineIndex
> > > to lock the constraint.
> >
> > table_open(childRelId, ...) would lock any "ALTER TABLE  DROP 
> > CONSTRAINT"
> > already. Not sure I understand your concern here.
> 
> I believe this is not true. This would take a lock on the table, not
> the constraint itself.

I agree that it would not lock the constraint itself. What I meant to say is 
that
, nevertheless, the constraint can not be dropped. Indeed, the "ALTER TABLE"
necessary to drop the constraint (ALTER TABLE  DROP CONSTRAINT) 
would
be locked by the table_open(childRelId, ...).

That's why I don't understand your concern with this particular example. But
anyway, I'll double check your related concern:

+ if (object->classId == RelationRelationId || object->classId ==
AuthMemRelationId)
+ return;

in depLockAndCheckObject(). 

Regards,

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

Re: [multithreading] extension compatibility

[Robert Haas]

On Thu, Jun 6, 2024 at 10:32 AM Heikki Linnakangas  wrote:
> > Well, OK, so it sounds like I'm outvoted, at least at the moment.
> > Maybe that will change as more people vote, but for now, that's where
> > we are. Given that, I suppose we want something more like Tristan's
> > patch, but with a more extensible syntax. Does that sound right?
>
> +1

So, how shall we proceed here? Tristan, do you want to update your
patch based on this feedback?

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

Re: Improve the granularity of PQsocketPoll's timeout parameter?

[Tom Lane]

Ranier Vilela  writes:
> +1 for push.

Done.  I noticed in final review that libpq-fe.h's "#include ",
which I'd feared to remove because I thought we'd shipped that
already, actually was new in f5e4dedfa.  So there shouldn't be
anything depending on it, and I thought it best to take it out again.
Widely-used headers shouldn't have unnecessary inclusions.

regards, tom lane

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Thu, 13 Jun 2024 at 17:19, Tom Lane  wrote:
> I wonder if we should be checking out some of the other newer
> languages that were mentioned upthread.

If this is actually something that we want to seriously evaluate, I
think that's a significant effort. And I think the people that want a
language would need to step in to make that effort. So far Jacob[1],
Alexander[2] and me[3] seem to be doing that for Python, and Sutou has
done that for Ruby[4].

[1]: https://github.com/pgbouncer/pgbouncer/tree/master/test
[2]: https://github.com/jchampio/pg-pytest-suite
[3]: https://github.com/postgrespro/testgres
[4]: 
https://github.com/pgroonga/pgroonga/blob/main/test/test-streaming-replication.rb

> It feels like going to
> Python here will lead to having two testing infrastructures with
> mas-o-menos the same capabilities, leaving us with a situation
> where people have to know both languages in order to make sense of
> our test suite.  I find it hard to picture that as an improvement
> over the status quo.

You don't have to be fluent in writing Python to be able to read and
understand tests written in it. As someone familiar with Python I can
definitely read our test suite, and I expect everyone smart enough to
be fluent in Perl to be able to read and understand Python with fairly
little effort too.

I think having significantly more tests being written, and those tests
being written faster and more correctly, is definitely worth the
slight mental effort of learning to read two very similarly looking
scripting languages (they both pretty much looking like pseudo code).

Re: strange context message in spi.c?

[Daniel Gustafsson]

> On 13 Jun 2024, at 19:21, Pavel Stehule  wrote:

> Is the message "SQL expression ..." for RAW_PLPGSQL_EXPR correct?

That indeed seems incorrect.

> Should there  be a "PL/pgSQL expression" instead?

I think that would make more sense.

--
Daniel Gustafsson

Re: Columnar format export in Postgres

[Sushrut Shivaswamy]

>
> If you want to have any hope, the license must be BSD.
> GPL is incompatible.


Ack, will update the license to BSD. Thanks

On Wed, Jun 12, 2024 at 10:49 PM Ranier Vilela  wrote:

> Em qua., 12 de jun. de 2024 às 13:56, Sushrut Shivaswamy <
> sushrut.shivasw...@gmail.com> escreveu:
>
>> Hey Postgres team,
>>
>> I have been working on adding support for columnar format export to
>> Postgres to speed up analytics queries.
>> I've created an extension that achieves this functionality here
>> .
>>
>> I"m looking to improve the performance of this extension to enable
>> drop-in analytics support for Postgres. Some immediate improvements I have
>> in mind are:
>>  - Reduce memory consumption when exporting table data to columnar format
>>  - Create a native planner / execution hook that can read columnar data
>> with vectorised operations.
>>
>> It would be very helpful if you could take a look and suggest
>> improvements to the extension.
>> Hopefully, this extension can be shipped by default with postgres at some
>> point in the future.
>>
> If you want to have any hope, the license must be BSD.
> GPL is incompatible.
>
> best regards,
> Ranier Vilela
>

Re: Conflict Detection and Resolution

[Robert Haas]

On Thu, May 23, 2024 at 2:37 AM shveta malik  wrote:
> c) update_deleted: The row with the same value as that incoming
> update's key does not exist. The row is already deleted. This conflict
> type is generated only if the deleted row is still detectable i.e., it
> is not removed by VACUUM yet. If the row is removed by VACUUM already,
> it cannot detect this conflict. It will detect it as update_missing
> and will follow the default or configured resolver of update_missing
> itself.

I think this design is categorically unacceptable. It amounts to
designing a feature that works except when it doesn't. I'm not exactly
sure how the proposal should be changed to avoid depending on the
timing of VACUUM, but I think it's absolutely not OK to depend on the
timing of VACUUm -- or, really, this is going to depend on the timing
of HOT-pruning, which will often happen almost instantly.

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

Re: Columnar format export in Postgres

[Sushrut Shivaswamy]

Thanks for the response.

I had considered using COPY TO to export columnar data but gave up on it
since the formats weren't extensible.
It's great to see that you are making it extensible.

I'm still going through the thread of comments on your patch but I have
some early thoughts about using it for columnar data export.

 - To maintain data freshness there would need to be a way to schedule
exports using `COPY TO 'parquet`` periodically
  - pg_analytica has the scheduling logic, once available COPY TO can
be used to export the data instead of reading table in chunks being used
currently.

 - To facilitate efficient querying it would help to export multiple
parquet files for the table instead of a single file.
   Having multiple files allows queries to skip chunks if the key range in
the chunk does not match query filter criteria.
   Even within a chunk it would help to be able to configure the size of a
row group.
  - I'm not sure how these parameters will be exposed within `COPY TO`.
Or maybe the extension implementing the `COPY TO` handler will
allow this configuration?

 - Regarding using file_fdw to read Apache Arrow and Apache Parquet file
because file_fdw is based on COPY FROM:
 - I'm not too clear on this. file_fdw seems to allow creating a table
from  data on disk exported using COPY TO.
   But is the newly created table still using the data on disk(maybe in
columnar format or csv) or is it just reading that data to create a row
based table.
   I'm not aware of any capability in the postgres planner to read
columnar files currently without using an extension like parquet_fdw.
- For your usecase how do you plan to query the arrow / parquet
data?

Re: Avoid orphaned objects dependencies, take 3

[Robert Haas]

On Thu, Jun 13, 2024 at 12:52 PM Bertrand Drouvot
 wrote:
> > > table_open(childRelId, ...) would lock any "ALTER TABLE  DROP 
> > > CONSTRAINT"
> > > already. Not sure I understand your concern here.
> >
> > I believe this is not true. This would take a lock on the table, not
> > the constraint itself.
>
> I agree that it would not lock the constraint itself. What I meant to say is 
> that
> , nevertheless, the constraint can not be dropped. Indeed, the "ALTER TABLE"
> necessary to drop the constraint (ALTER TABLE  DROP CONSTRAINT) 
> would
> be locked by the table_open(childRelId, ...).

Ah, right. So, I was assuming that, with either this version of your
patch or the earlier version, we'd end up locking the constraint
itself. Was I wrong about that?

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

Re: RFC: adding pytest as a supported test framework

[Melanie Plageman]

On Thu, Jun 13, 2024 at 7:27 AM Daniel Gustafsson  wrote:
>
> > On 13 Jun 2024, at 00:34, Melanie Plageman  
> > wrote:
>
> > FWIW, I felt a lot of pain trying to write recovery TAP tests with
> > IPC::Run's pumping functionality. It was especially painful (as
> > someone who knows even less Perl than the "street fighting Perl"
> > Thomas Munro has described having) before the additional test
> > infrastructure was added in BackgroudPsql.pm last year.
>
> A key aspect of this, which isn't specific to Perl or our use of it, is that
> this was done in backbranches which doesn't have the (recently) much improved
> BackgroundPsql.pm.  The quality of our tools and the ease of use they provide
> is directly related to the investment we make into continuously improving our
> testharness.  Regardless of which toolset we adopt, if we don't make this
> investment (taking learnings from the past years and toolsets into account)
> we're bound to repeat this thread in a few years advocating for toolset X+1.

True. And thank you for committing BackgroundPsql.pm (and Andres for
starting that work). My specific case is likely one of a poor work
person blaming her tools :)

- Melanie

Re: SQL function which allows to distinguish a server being in point in time recovery mode and an ordinary replica

[m . litsarev]

Hi!

Michael,
I have fixed the patches according to your comments.


merge both local variables into a single bits32 store?

This is done in v3-0001-Standby-mode-requested.patch


Then this could be used with a function that returns a
text[] array with all the states retrieved?

Placed this in the v3-0002-Text-array-sql-wrapper.patch


The refactoring pieces and the function pieces should be split, for
clarity.
Sure. I also added the third patch with some tests. Perhaps it would be 
usefull.


Respectfully,

Mikhail Litsarev
Postgres Professional: https://postgrespro.com
From cd32de9ec14dba0011f8c05e6a8ab7e4f7be045c Mon Sep 17 00:00:00 2001
From: Mikhail Litsarev 
Date: Thu, 13 Jun 2024 18:47:47 +0300
Subject: [PATCH] Tests for standby-is-requested mode.

Add tiny tests in src/test/recovery/t/004_timeline_switch.pl
They validate:
 - master is not a replica
 - standby_1 is a replica
 - promoted replica in master mode
 - standby_2 remains a replica after switch to promoted master
---
 src/test/recovery/t/004_timeline_switch.pl | 25 ++
 1 file changed, 25 insertions(+)

diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 684838cbab7..11f0b22ae90 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -16,6 +16,11 @@ my $node_primary = PostgreSQL::Test::Cluster->new('primary');
 $node_primary->init(allows_streaming => 1);
 $node_primary->start;
 
+# Validate pg_get_recovery_flags() for master
+my $ret_mode_primary = $node_primary->safe_psql('postgres',
+	'SELECT pg_get_recovery_flags()');
+is($ret_mode_primary, '{}', "master is not a replica");
+
 # Take backup
 my $backup_name = 'my_backup';
 $node_primary->backup($backup_name);
@@ -30,6 +35,12 @@ $node_standby_2->init_from_backup($node_primary, $backup_name,
 	has_streaming => 1);
 $node_standby_2->start;
 
+# Validate standby-mode-requested state for replica node
+my $ret_mode_standby_1 = $node_standby_1->safe_psql('postgres',
+	'SELECT pg_get_recovery_flags()');
+is($ret_mode_standby_1, '{STANDBY_MODE_REQUESTED}', "node_standby_1 is a replica");
+print($ret_mode_standby_1);
+
 # Create some content on primary
 $node_primary->safe_psql('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
@@ -48,6 +59,15 @@ $node_standby_1->psql(
 	stdout => \$psql_out);
 is($psql_out, 't', "promotion of standby with pg_promote");
 
+# Validate pg_get_recovery_flags() for master promoted from standby node.
+# STANDBY_MODE_REQUESTED is returned because standby.signal file
+# was found while being a replica.
+# Use it with pg_is_in_recovery() (will return false), for such use-cases.
+my $ret_mode_1 = $node_standby_1->safe_psql('postgres',
+	'SELECT pg_get_recovery_flags()');
+is($ret_mode_1, '{PROMOTE_IS_TRIGGERED,STANDBY_MODE_REQUESTED}',
+	"node_standby_1 becomes a master");
+
 # Switch standby 2 to replay from standby 1
 my $connstr_1 = $node_standby_1->connstr;
 $node_standby_2->append_conf(
@@ -56,6 +76,11 @@ primary_conninfo='$connstr_1'
 ));
 $node_standby_2->restart;
 
+# Validate STANDBY_MODE_REQUESTED for second replica after restart
+my $ret_mode_standby_2 = $node_standby_2->safe_psql('postgres',
+	'SELECT pg_get_recovery_flags()');
+is($ret_mode_standby_2, '{STANDBY_MODE_REQUESTED}', "node_standby_2 remains a replica");
+
 # Insert some data in standby 1 and check its presence in standby 2
 # to ensure that the timeline switch has been done.
 $node_standby_1->safe_psql('postgres',
-- 
2.34.1

From ac304f75d5f6f9bb9712ef4113da035ad74b7344 Mon Sep 17 00:00:00 2001
From: Mikhail Litsarev 
Date: Thu, 13 Jun 2024 18:37:13 +0300
Subject: [PATCH] Wrapper function to extract whole text array from the
 SharedRecoveryDataFlags.

---
 src/backend/access/transam/xlogfuncs.c| 31 +++
 src/backend/access/transam/xlogrecovery.c | 17 +
 src/include/access/xlogrecovery.h |  1 +
 src/include/catalog/pg_proc.dat   |  4 +++
 4 files changed, 53 insertions(+)

diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 4e46baaebdf..9e752639aa4 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -29,6 +29,7 @@
 #include "replication/walreceiver.h"
 #include "storage/fd.h"
 #include "storage/standby.h"
+#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/memutils.h"
 #include "utils/pg_lsn.h"
@@ -747,3 +748,33 @@ pg_promote(PG_FUNCTION_ARGS)
 		   wait_seconds)));
 	PG_RETURN_BOOL(false);
 }
+
+Datum
+pg_get_recovery_flags(PG_FUNCTION_ARGS)
+{
+/*
+ * Currently supported number of recovery flags is equal to 2:
+ * {XLR_PROMOTE_IS_TRIGGERED, XLR_STANDBY_MODE_REQUESTED}.
+ * It can be extended in future.
+ */
+#define MAX_RECOVERY_FLAGS 2
+
+	bits32		recovery_flags;
+	int			cnt = 0;
+	Datum		flags[MAX_RECOVERY_FLAGS];
+	ArrayType	*txt_arr;
+
+	recovery_flags = 

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Thu, Jun 13, 2024 at 11:19 AM Tom Lane  wrote:
> I wonder if we should be checking out some of the other newer
> languages that were mentioned upthread.  It feels like going to
> Python here will lead to having two testing infrastructures with
> mas-o-menos the same capabilities, leaving us with a situation
> where people have to know both languages in order to make sense of
> our test suite.  I find it hard to picture that as an improvement
> over the status quo.

As I see it, one big problem is that if you pick a language that's too
new, it's more likely to fade away. Python is very well-established,
e.g. see

https://www.tiobe.com/tiobe-index/

That gives Python a rating of 15.39%; vs. Perl at 0.69%. There are
other things that you could pick, for sure, like Javascript, but if
you want a scripting language that's popular now, Python is hard to
beat. And that means it's more likely to still have some life in it 10
or 20 years from now than many other things.

Not all sites agree on which programming languages are actually the
most popular and I'm not strongly against considering other
possibilities, but Python seems to be pretty high on most lists, often
#1, and that does matter.

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

Re: Conflict Detection and Resolution

[Jonathan S. Katz]

On 6/13/24 7:28 AM, Amit Kapila wrote:


You are right that users would wish to detect the conflicts and
probably the extra effort would only be in the 'update_differ' case
where we need to consult committs module and that we will only do when
'track_commit_timestamp' is true. BTW, I think for Inserts with
primary/unique key violation, we should catch the ERROR and log it. If
we want to log the conflicts in a separate table then do we want to do
that in the catch block after getting pk violation or do an extra scan
before 'INSERT' to find the conflict? I think logging would need extra
cost especially if we want to LOG it in some table as you are
suggesting below that may need some option.


Therefore, additional conflict detection for these cases is currently
omitted to minimize potential overhead. However, the pre-detection for
conflict in these error cases is still essential to support automatic
conflict resolution in the future.


I feel that we should log all types of conflict in an uniform way. For
example, with detect_conflict being true, the update_differ conflict
is reported as "conflict %s detected on relation "%s"", whereas
concurrent inserts with the same key is reported as "duplicate key
value violates unique constraint "%s"", which could confuse users.
Ideally, I think that we log such conflict detection details (table
name, column name, conflict type, etc) to somewhere (e.g. a table or
server logs) so that the users can resolve them manually.



It is good to think if there is a value in providing in
pg_conflicts_history kind of table which will have details of
conflicts that occurred and then we can extend it to have resolutions.
I feel we can anyway LOG the conflicts by default. Updating a separate
table with conflicts should be done by default or with a knob is a
point to consider.


+1 for logging conflicts uniformly, but I would +100 to exposing the log 
in a way that's easy for the user to query (whether it's a system view 
or a stat table). Arguably, I'd say that would be the most important 
feature to come out of this effort.


Removing how conflicts are resolved, users want to know exactly what row 
had a conflict, and users from other database systems that have dealt 
with these issues will have tooling to be able to review and analyze if 
a conflicts occur. This data is typically stored in a queryable table, 
with data retained for N days. When you add in automatic conflict 
resolution, users then want to have a record of how the conflict was 
resolved, in case they need to manually update it.


Having this data in a table also gives the user opportunity to 
understand conflict stats (e.g. conflict rates) and potentially identify 
portions of the application and other parts of the system to optimize. 
It also makes it easier to import to downstream systems that may perform 
further analysis on conflict resolution, or alarm if a conflict rate 
exceeds a certain threshold.


Thanks,

Jonathan




OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: RFC: adding pytest as a supported test framework

[Tom Lane]

Jelte Fennema-Nio  writes:
> You don't have to be fluent in writing Python to be able to read and
> understand tests written in it.

[ shrug... ]  I think the same can be said of Perl, with about as
much basis.  It matters a lot if you have previous experience with
the language.

regards, tom lane

Re: RFC: adding pytest as a supported test framework

[Tom Lane]

Robert Haas  writes:
> On Wed, Jun 12, 2024 at 6:43 PM Jelte Fennema-Nio  wrote:
>> I agree it's not a technical issue. It is a people issue. There are
>> very few people skilled in Perl active in the community. And most of
>> those are very senior hackers that have much more important things to
>> do that make our Perl testing framework significantly better. And the
>> less senior people that might see improving tooling as a way to get
>> help out in the community, are try to stay away from Perl with a 10
>> foot pole. So the result is, nothing gets improved. Especially since
>> very few people outside our community improve this tooling either.

> I agree with you, but I'm skeptical that solving it will be as easy as
> switching to Python. For whatever reason, it seems like every piece of
> infrastructure that the PostgreSQL community has suffers from severe
> neglect.

Yeah.  In this case it's perhaps more useful to look at our external
dependencies, the large majority of which are suffering from age
and neglect:

  * autoconf & gmake (although meson may get us out from under these)
  * bison
  * flex
  * perl
  * tcl
  * regex library (basically from tcl)
  * libxml2
  * kerberos
  * ldap
  * pam
  * uuid library

I think the basic problem is inherent in being a successful long-lived
project.  Or maybe we're just spectacularly bad at picking which
things to depend on.  Whichever it is, we'd better have a 10- or 20-
year perspective when thinking about adopting new major dependencies.

In the case at hand, I share Robert's doubts about Python.  Sure it's
more popular than Perl, but I don't think it's actually better, and
in some ways it's worse.  (The moving-target package collection was
mentioned as a problem, for instance.)  Is it going to age better
than Perl?  Doubt it.

I wonder if we should be checking out some of the other newer
languages that were mentioned upthread.  It feels like going to
Python here will lead to having two testing infrastructures with
mas-o-menos the same capabilities, leaving us with a situation
where people have to know both languages in order to make sense of
our test suite.  I find it hard to picture that as an improvement
over the status quo.

regards, tom lane

Re: libpq contention due to gss even when not using gss

[Andres Freund]

Hi,

On 2024-06-13 17:33:57 +0200, Dmitry Dolgov wrote:
> > On Mon, Jun 10, 2024 at 11:12:12AM GMT, Andres Freund wrote:
> > Hi,
> >
> > To investigate a report of both postgres and pgbouncer having issues when a
> > lot of new connections aree established, I used pgbench -C.  Oddly, on an
> > early attempt, the bottleneck wasn't postgres+pgbouncer, it was pgbench. But
> > only when using TCP, not with unix sockets.
> >
> > c=40;pgbench -C -n -c$c -j$c -T5 -f <(echo 'select 1') 'port=6432 
> > host=127.0.0.1 user=test dbname=postgres password=fake'
> >
> > host=127.0.0.1:   16465
> > host=127.0.0.1,gssencmode=disable 20860
> > host=/tmp:49286
> >
> > Note that the server does *not* support gss, yet gss has a substantial
> > performance impact.
> >
> > Obviously the connection rates here absurdly high and outside of badly 
> > written
> > applications likely never practically relevant. However, the number of cores
> > in systems are going up, and this quite possibly will become relevant in 
> > more
> > realistic scenarios (lock contention kicks in earlier the more cores you
> > have).
> 
> By not supporting gss I assume you mean having built with --with-gssapi,
> but only host (not hostgssenc) records in pg_hba, right?

Yes, the latter. Or not having kerberos set up on the client side.

Greetings,

Andres Freund

Re: MultiXactMemberFreezeThreshold can make autovacuum *less* aggressive

[Heikki Linnakangas]

On 13/06/2024 17:40, Robert Haas wrote:

On Thu, Jun 13, 2024 at 8:29 AM Heikki Linnakangas  wrote:

However, the value that the function calculates can sometimes be
*greater* than autovacuum_multixact_freeze_max_age.


That was definitely not what I intended and is definitely bad.


In any case, I think the function
should clamp the result to autovacuum_multixact_freeze_max_age, per
attached.


LGTM.


Committed and backpatched to all supported versions. Thanks!

--
Heikki Linnakangas
Neon (https://neon.tech)

strange context message in spi.c?

[Pavel Stehule]

Hi

I am found strange switch:

<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}

Is the message "SQL expression ..." for RAW_PLPGSQL_EXPR correct?

Should there  be a "PL/pgSQL expression" instead?

Regards

Pavel

Re: Improve the granularity of PQsocketPoll's timeout parameter?

[Ranier Vilela]

Em qui., 13 de jun. de 2024 às 12:25, Tom Lane  escreveu:

> Ranier Vilela  writes:
> > I'm unsure if the documentation matches the code?
> > " connect_timeout #
> > <
> https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-CONNECT-TIMEOUT
> >
>
> > Maximum time to wait while connecting, in seconds (write as a decimal
> > integer, e.g., 10). Zero, negative, or not specified means wait
> > indefinitely. The minimum allowed timeout is 2 seconds, therefore a value
> > of 1 is interpreted as 2. This timeout applies separately to each host
> name
> > or IP address. For example, if you specify two hosts and connect_timeout
> is
> > 5, each host will time out if no connection is made within 5 seconds, so
> > the total time spent waiting for a connection might be up to 10 seconds.
> > "
> > The comments says that timeout = 0, means *Timeout is immediate (no
> > blocking)*
>
> > Does the word "indefinitely" mean infinite?
> > If yes, connect_timeout = -1, mean infinite?
>
> The sentence about "minimum allowed timeout is 2 seconds" has to go
> away, but the rest of that seems fine.
>
> But now that you mention it, we could drop the vestigial
>
> >> if (timeout < 0)
> >> timeout = 0;
>
> as well, because the rest of the function only applies the timeout
> when "timeout > 0".  Otherwise end_time (nee finish_time) stays at -1.
>
I think that's OK Tom.

+1 for push.

best regards,
Ranier Vilela

Re: jsonpath: Missing Binary Execution Path?

[David E. Wheeler]

On Jun 13, 2024, at 11:32, David E. Wheeler  wrote:

> Should && and || not also work on scalar operands?

I see the same issue for unary !, too:

david=# select jsonb_path_query('true', '!$');
ERROR:  syntax error at or near "$" of jsonpath input
LINE 1: select jsonb_path_query('true', '!$');
^
david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (!true)');
ERROR:  syntax error at end of jsonpath input
LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (!true)');
 ^
david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (!@.boolean())');
ERROR:  syntax error at or near "@" of jsonpath input
LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (!@.boolean())'...
 ^

Best,

David

Re: MultiXactMemberFreezeThreshold can make autovacuum *less* aggressive

[Robert Haas]

On Thu, Jun 13, 2024 at 8:29 AM Heikki Linnakangas  wrote:
> However, the value that the function calculates can sometimes be
> *greater* than autovacuum_multixact_freeze_max_age.

That was definitely not what I intended and is definitely bad.

> In any case, I think the function
> should clamp the result to autovacuum_multixact_freeze_max_age, per
> attached.

LGTM.

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

jsonpath: Missing Binary Execution Path?

[David E. Wheeler]

Hackers,

Another apparent inconsistency I’ve noticed in jsonpath queries is the 
treatment of the && and || operators: They can’t operate on scalar functions, 
only on other expressions. Some examples:

david=# select jsonb_path_query('true', '$ && $');
ERROR:  syntax error at or near "&&" of jsonpath input
LINE 1: select jsonb_path_query('true', '$ && $');
^
david=# select jsonb_path_query('true', '$.boolean() && $.boolean()');
ERROR:  syntax error at or near "&&" of jsonpath input
LINE 1: select jsonb_path_query('true', '$.boolean() && $.boolean()'...
^
The only place I’ve seen them work is inside filters with binary or unary 
operands:

jsonb_path_query('[1, 3, 7]', '$[*] ? (@ > 1 && @ < 5)');
 jsonb_path_query 
--
 3

It doesn’t even work with boolean methods!

david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (@.boolean() && 
@.boolean())');
ERROR:  syntax error at or near "&&" of jsonpath input
LINE 1: select jsonb_path_query('[1, 3, 7]', '$[*] ? (@.boolean() &&...
 ^
Other binary operators work just fine in these sorts of contexts:

david=# select jsonb_path_query('1', '$ >= 1');
 jsonb_path_query 
--
 true
(1 row)

david=# select jsonb_path_query('[1, 3, 7]', '$[*] ? (@ > 1)');
 jsonb_path_query 
--
 3
 7
(2 rows)

Should && and || not also work on scalar operands?

Best,

David

Re: Avoid orphaned objects dependencies, take 3

[Robert Haas]

On Fri, Jun 7, 2024 at 4:41 AM Bertrand Drouvot
 wrote:
> Do you still find the code hard to maintain with v9?

I don't think it substantially changes my concerns as compared with
the earlier version.

> > but we're not similarly careful about other operations e.g.
> > ConstraintSetParentConstraint is called by DefineIndex which calls
> > table_open(childRelId, ...) first, but there's no logic in DefineIndex
> > to lock the constraint.
>
> table_open(childRelId, ...) would lock any "ALTER TABLE  DROP 
> CONSTRAINT"
> already. Not sure I understand your concern here.

I believe this is not true. This would take a lock on the table, not
the constraint itself.

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

Re: libpq contention due to gss even when not using gss

[Dmitry Dolgov]

> On Mon, Jun 10, 2024 at 11:12:12AM GMT, Andres Freund wrote:
> Hi,
>
> To investigate a report of both postgres and pgbouncer having issues when a
> lot of new connections aree established, I used pgbench -C.  Oddly, on an
> early attempt, the bottleneck wasn't postgres+pgbouncer, it was pgbench. But
> only when using TCP, not with unix sockets.
>
> c=40;pgbench -C -n -c$c -j$c -T5 -f <(echo 'select 1') 'port=6432 
> host=127.0.0.1 user=test dbname=postgres password=fake'
>
> host=127.0.0.1:   16465
> host=127.0.0.1,gssencmode=disable 20860
> host=/tmp:49286
>
> Note that the server does *not* support gss, yet gss has a substantial
> performance impact.
>
> Obviously the connection rates here absurdly high and outside of badly written
> applications likely never practically relevant. However, the number of cores
> in systems are going up, and this quite possibly will become relevant in more
> realistic scenarios (lock contention kicks in earlier the more cores you
> have).

By not supporting gss I assume you mean having built with --with-gssapi,
but only host (not hostgssenc) records in pg_hba, right?

Re: Improve the granularity of PQsocketPoll's timeout parameter?

[Tom Lane]

Ranier Vilela  writes:
> I'm unsure if the documentation matches the code?
> " connect_timeout #
> 

> Maximum time to wait while connecting, in seconds (write as a decimal
> integer, e.g., 10). Zero, negative, or not specified means wait
> indefinitely. The minimum allowed timeout is 2 seconds, therefore a value
> of 1 is interpreted as 2. This timeout applies separately to each host name
> or IP address. For example, if you specify two hosts and connect_timeout is
> 5, each host will time out if no connection is made within 5 seconds, so
> the total time spent waiting for a connection might be up to 10 seconds.
> "
> The comments says that timeout = 0, means *Timeout is immediate (no
> blocking)*

> Does the word "indefinitely" mean infinite?
> If yes, connect_timeout = -1, mean infinite?

The sentence about "minimum allowed timeout is 2 seconds" has to go
away, but the rest of that seems fine.

But now that you mention it, we could drop the vestigial

>> if (timeout < 0)
>> timeout = 0;

as well, because the rest of the function only applies the timeout
when "timeout > 0".  Otherwise end_time (nee finish_time) stays at -1.

regards, tom lane

Re: Contributing test cases to improve coverage

[Peter Eisentraut]

On 12.06.24 18:44, J F wrote:
(a) The regression test suite is run by a parallel scheduler, with some 
test cases dependent on previous test cases. If I just add my test case 
as part of the parallel scheduler’s tests, it might not work, since 
previous test cases in the scheduler might already create the same 
table, for instance.


Yes, you need to take care of that somehow.  Some test files put all 
their test objects in a schema.  Others are careful to drop all test 
objects at the end.  Or you just have to pick non-conflicting names.


(b) How do I get my test cases reviewed and ultimately included in a 
future release of PostgreSQL?


Perhaps start with

https://wiki.postgresql.org/wiki/Development_information

and in particular

https://wiki.postgresql.org/wiki/Submitting_a_Patch

Re: Contributing test cases to improve coverage

[Ashutosh Bapat]

On Thu, Jun 13, 2024 at 1:22 AM J F  wrote:

> > I am quite confused about what is the point of this.  You have not
> > found any actual bug, nor have you demonstrated that this test case
> > could discover a likely future bug that wouldn't be detected another
> > way.  Moreover, it seems like the process would lead to some very
> > large number of equally marginal test cases.  We aren't likely to
> > accept such a patch, because we are concerned about keeping down the
> > runtime of the test suite.
> >
> >regards, tom lane
>
>
> The point of this project is to improve the coverage of PostgreSQL’s
> preexisting test suite. Writing a test suite to achieve close to 100%
> coverage is challenging, but I have proposed a workflow to automate this
> process.
>

We monitor code coverage. But this is doing more than that. It will find
out the places in code, which if changed, will cause bugs. That seems
useful to avoid refactoring mistakes, esp. when people rely on regression
tests to tell whether their code changes are sane. But in PostgreSQL, we
rely heavily on reviewers and committers to do that instead of tests.
Still, the tests produced by this tool will help catch bugs that human eyes
can not. As Tom said, managing that battery of tests may not be worth it.
Basically, I am flip-flopping on the usefulness of this effort.


>
> I assert that no test case in the regression test suite currently covers
> the comparator in the expression rte->rtekind == RTE_SUBQUERY. I propose
> adding a new test case that addresses exactly this. In the future, if
> someone accidentally modifies the operator to become >=, it will trigger
> incorrect behavior when certain queries are executed. This test case will
> catch that issue.
>

Usually PostgreSQL developers know that rtekind is an enum so they are very
very unlikely to use anything other than == and !=. Such a change will be
caught by a reviewer. I think many of the tests that this tool will produce
will fall in this category.


>
> I get that the test cases in /regress are likely reserved for actual bugs
> found and are designed to run quickly. Would it be a good idea to have a
> separate, more rigorous test suite that runs longer but provides better
> code coverage?
>
> There are practical difficulties like maintaining the expected outputs for
such a large battery of tests. But maybe some external project could.

BTW, have you considered perl tests, isolation tests etc. Tests in regress/
do not cover many subsystems e.g. replication.
-- 
Best Wishes,
Ashutosh Bapat

Re: Keeping track of buildfarm animals' personality

[Alvaro Herrera]

On 2024-Jun-12, Thomas Munro wrote:

> On Wed, Jun 12, 2024 at 4:13 AM Andrew Dunstan  wrote:

> > There are two problems with this system, and have been since the get-go.
> > First, owners forget to run the upgrade-personalty.pl script when the
> > upgrade the OS and./or compiler. Second, there is no standardization of
> > the names. Complaints about these surfaced (again) at the Vancouver
> > unconference session about the buildfarm's future.
> 
> I was sorry to miss that one!  Basically every unconference was
> unmissable, but you had to miss 3/4 of them due to physics...

2/3.  Yeah, but I think the room selection algorithm we used is flawed.
One that works better schedules all the most popular talks in the
biggest room (matches what we did), but for the second room, we should
sort in the opposite direction: put the fifth most popular topic in the
last slot of the second room, and the sixth most popular in the last
slot of the third room (instead, we filled the second and third room the
same way as the first one).  So for twelve topics it'd be like this,
numbers represent topics ordered by popularity:

room 1  room 2  room 3
  1. 11. 12.
  2.  9. 10.
  3.  7.  8.
  4.  5.  6.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Wed, Jun 12, 2024 at 6:43 PM Jelte Fennema-Nio  wrote:
> I agree it's not a technical issue. It is a people issue. There are
> very few people skilled in Perl active in the community. And most of
> those are very senior hackers that have much more important things to
> do that make our Perl testing framework significantly better. And the
> less senior people that might see improving tooling as a way to get
> help out in the community, are try to stay away from Perl with a 10
> foot pole. So the result is, nothing gets improved. Especially since
> very few people outside our community improve this tooling either.

I agree with you, but I'm skeptical that solving it will be as easy as
switching to Python. For whatever reason, it seems like every piece of
infrastructure that the PostgreSQL community has suffers from severe
neglect. Literally everything I know of either has one or maybe two
very senior hackers maintaining it, or no maintainer at all. Andrew
maintains the buildfarm and it evolves quite slowly. Andres did all
the work on meson, with some help from Peter. Thomas maintains cfbot
as a skunkworks. The Perl-based TAP test framework gets barely any
love at all. The CommitFest application is pretty much totally
stagnant, and in fact is a great example of what I'm talking about
here: I wrote an original version in Perl and somebody -- I think
Magnus -- rewrote it in a more maintainable framework -- and then the
development pace went to basically zero. All of this stuff is critical
project infrastructure and yet it feels like nobody wants to work on
it.

Now, this case may prove to be an exception to that rule and that will
be great. But what I think is a lot more likely is that we'll get a
lot of pressure to commit something as soon as parity with the Perl
TAP test system has been achieved, or maybe even before that, and then
the rate of further improvements will slow to a trickle. That's not to
say that sticking with Perl is better. A quick Google search finds a
web page that says Python is two orders of magnitude more popular than
Perl, and that's not something we should just ignore. But I still
think it's fair to question whether the preference of many developers
for Python over Perl will translate into sustained investment in
improving the infrastructure. Again, I will be thrilled if it does,
but that just doesn't seem to be the way that things go around here,
and I bet the reasons go well beyond choice of programming language.

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

Re: Conflict Detection and Resolution

[Alvaro Herrera]

On 2024-Jun-07, Tomas Vondra wrote:

> On 6/3/24 09:30, Amit Kapila wrote:
> > On Sat, May 25, 2024 at 2:39 AM Tomas Vondra 
> >  wrote:

> >> How is this going to deal with the fact that commit LSN and timestamps
> >> may not correlate perfectly? That is, commits may happen with LSN1 <
> >> LSN2 but with T1 > T2.

> But as I wrote, I'm not quite convinced this means there are not other
> issues with this way of resolving conflicts. It's more likely a more
> complex scenario is required.

Jan Wieck approached me during pgconf.dev to reproach me of this
problem.  He also said he had some code to fix-up the commit TS
afterwards somehow, to make the sequence monotonically increasing.
Perhaps we should consider that, to avoid any problems caused by the
difference between LSN order and TS order.   It might be quite
nightmarish to try to make the system work correctly without
reasonable constraints of that nature.

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

Re: Conflict Detection and Resolution

[Peter Eisentraut]

On 23.05.24 08:36, shveta malik wrote:

Conflict Resolution

a) latest_timestamp_wins:The change with later commit timestamp wins.
b) earliest_timestamp_wins:   The change with earlier commit timestamp wins.
c) apply:   Always apply the remote change.
d) skip:Remote change is skipped.
e) error:   Error out on conflict. Replication is stopped, manual
action is needed.


You might be aware of pglogical, which has similar conflict resolution 
modes, but they appear to be spelled a bit different.  It might be worth 
reviewing this, so that we don't unnecessarily introduce differences.


https://github.com/2ndquadrant/pglogical?tab=readme-ov-file#conflicts

There might also be other inspiration to be found related to this in 
pglogical documentation or code.

Re: Logical Replication of sequences

[Masahiko Sawada]

On Thu, Jun 13, 2024 at 7:06 PM Amit Kapila  wrote:
>
> On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada  wrote:
> >
> > On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila  wrote:
> > >
> > >
> > > Yeah, starting with a single worker sounds good for now. Do you think
> > > we should sync all the sequences in a single transaction or have some
> > > threshold value above which a different transaction would be required
> > > or maybe a different sequence sync worker altogether? Now, having
> > > multiple sequence-sync workers requires some synchronization so that
> > > only a single worker is allocated for one sequence.
> > >
> > > The simplest thing is to use a single sequence sync worker that syncs
> > > all sequences in one transaction but with a large number of sequences,
> > > it could be inefficient. OTOH, I am not sure if it would be a problem
> > > in reality.
> >
> > I think that we can start with using a single worker and one
> > transaction, and measure the performance with a large number of
> > sequences.
> >
>
> Fair enough. However, this raises the question Dilip and Vignesh are
> discussing whether we need a new relfilenode for sequence update even
> during initial sync? As per my understanding, the idea is that similar
> to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
> will create the new sequence entries in pg_subscription_rel with the
> state as 'i'. Then the sequence-sync worker would start a transaction
> and one-by-one copy the latest sequence values for each sequence (that
> has state as 'i' in pg_subscription_rel) and mark its state as ready
> 'r' and commit the transaction. Now if there is an error during this
> operation it will restart the entire operation. The idea of creating a
> new relfilenode is to handle the error so that if there is a rollback,
> the sequence state will be rolled back to 'i' and the sequence value
> will also be rolled back. The other option could be that we update the
> sequence value without a new relfilenode and if the transaction rolled
> back then only the sequence's state will be rolled back to 'i'. This
> would work with a minor inconsistency that sequence values will be
> up-to-date even when the sequence state is 'i' in pg_subscription_rel.
> I am not sure if that matters because anyway, they can quickly be
> out-of-sync with the publisher again.

I think it would be fine in many cases even if the sequence value is
up-to-date even when the sequence state is 'i' in pg_subscription_rel.
But the case we would like to avoid is where suppose the sequence-sync
worker does both synchronizing sequence values and updating the
sequence states for all sequences in one transaction, and if there is
an error we end up retrying the synchronization for all sequences.

>
> Now, say we don't want to maintain the state of sequences for initial
> sync at all then after the error how will we detect if there are any
> pending sequences to be synced? One possibility is that we maintain a
> subscription level flag 'subsequencesync' in 'pg_subscription' to
> indicate whether sequences need sync. This flag would indicate whether
> to sync all the sequences in pg_susbcription_rel. This would mean that
> if there is an error while syncing the sequences we will resync all
> the sequences again. This could be acceptable considering the chances
> of error during sequence sync are low. The benefit is that both the
> REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
> idea and sync all sequences without needing a new relfilenode. Users
> can always refer 'subsequencesync' flag in 'pg_subscription' to see if
> all the sequences are synced after executing the command.

I think that REFRESH PUBLICATION {SEQUENCES} can be executed even
while the sequence-sync worker is synchronizing sequences. In this
case, the worker might not see new sequences added by the concurrent
REFRESH PUBLICATION {SEQUENCES} command since it's already running.
The worker could end up marking the subsequencesync as completed while
not synchronizing these new sequences.

>
> > > > Or yet another idea I came up with is that a tablesync worker will
> > > > synchronize both the table and sequences owned by the table. That is,
> > > > after the tablesync worker caught up with the apply worker, the
> > > > tablesync worker synchronizes sequences associated with the target
> > > > table as well. One benefit would be that at the time of initial table
> > > > sync being completed, the table and its sequence data are consistent.
> >
> > Correction; it's not guaranteed that the sequence data and table data
> > are consistent even in this case since the tablesync worker could get
> > on-disk sequence data that might have already been updated.
> >
>
> The benefit of this approach is not clear to me. Our aim is to sync
> all sequences before the upgrade, so not sure if this helps because
> anyway both table values and corresponding sequences can again be
> out-of-sync very quickly.

Right.

Re: Logical Replication of sequences

[Amit Kapila]

On Thu, Jun 13, 2024 at 1:09 PM Masahiko Sawada  wrote:
>
> On Wed, Jun 12, 2024 at 6:59 PM Amit Kapila  wrote:
> >
> >
> > Yeah, starting with a single worker sounds good for now. Do you think
> > we should sync all the sequences in a single transaction or have some
> > threshold value above which a different transaction would be required
> > or maybe a different sequence sync worker altogether? Now, having
> > multiple sequence-sync workers requires some synchronization so that
> > only a single worker is allocated for one sequence.
> >
> > The simplest thing is to use a single sequence sync worker that syncs
> > all sequences in one transaction but with a large number of sequences,
> > it could be inefficient. OTOH, I am not sure if it would be a problem
> > in reality.
>
> I think that we can start with using a single worker and one
> transaction, and measure the performance with a large number of
> sequences.
>

Fair enough. However, this raises the question Dilip and Vignesh are
discussing whether we need a new relfilenode for sequence update even
during initial sync? As per my understanding, the idea is that similar
to tables, the CREATE SUBSCRIPTION command (with copy_data = true)
will create the new sequence entries in pg_subscription_rel with the
state as 'i'. Then the sequence-sync worker would start a transaction
and one-by-one copy the latest sequence values for each sequence (that
has state as 'i' in pg_subscription_rel) and mark its state as ready
'r' and commit the transaction. Now if there is an error during this
operation it will restart the entire operation. The idea of creating a
new relfilenode is to handle the error so that if there is a rollback,
the sequence state will be rolled back to 'i' and the sequence value
will also be rolled back. The other option could be that we update the
sequence value without a new relfilenode and if the transaction rolled
back then only the sequence's state will be rolled back to 'i'. This
would work with a minor inconsistency that sequence values will be
up-to-date even when the sequence state is 'i' in pg_subscription_rel.
I am not sure if that matters because anyway, they can quickly be
out-of-sync with the publisher again.

Now, say we don't want to maintain the state of sequences for initial
sync at all then after the error how will we detect if there are any
pending sequences to be synced? One possibility is that we maintain a
subscription level flag 'subsequencesync' in 'pg_subscription' to
indicate whether sequences need sync. This flag would indicate whether
to sync all the sequences in pg_susbcription_rel. This would mean that
if there is an error while syncing the sequences we will resync all
the sequences again. This could be acceptable considering the chances
of error during sequence sync are low. The benefit is that both the
REFRESH PUBLICATION SEQUENCES and CREATE SUBSCRIPTION can use the same
idea and sync all sequences without needing a new relfilenode. Users
can always refer 'subsequencesync' flag in 'pg_subscription' to see if
all the sequences are synced after executing the command.

> > > Or yet another idea I came up with is that a tablesync worker will
> > > synchronize both the table and sequences owned by the table. That is,
> > > after the tablesync worker caught up with the apply worker, the
> > > tablesync worker synchronizes sequences associated with the target
> > > table as well. One benefit would be that at the time of initial table
> > > sync being completed, the table and its sequence data are consistent.
>
> Correction; it's not guaranteed that the sequence data and table data
> are consistent even in this case since the tablesync worker could get
> on-disk sequence data that might have already been updated.
>

The benefit of this approach is not clear to me. Our aim is to sync
all sequences before the upgrade, so not sure if this helps because
anyway both table values and corresponding sequences can again be
out-of-sync very quickly.

> >
> > > >
> > > > 3) Refreshing the sequence can be achieved through the existing
> > > > command: ALTER SUBSCRIPTION ... REFRESH PUBLICATION(no syntax change
> > > > here).
> > > > The subscriber identifies stale sequences, meaning sequences present
> > > > in pg_subscription_rel but absent from the publication, and removes
> > > > them from the pg_subscription_rel system table. The subscriber also
> > > > checks for newly added sequences in the publisher and synchronizes
> > > > their values from the publisher using the steps outlined in the
> > > > subscription creation process. It's worth noting that previously
> > > > synchronized sequences won't be synchronized again; the sequence sync
> > > > will occur solely for the newly added sequences.
> > > >
> > > > 4) Introducing a new command for refreshing all sequences: ALTER
> > > > SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES.
> > > > The subscriber will remove stale sequences and add newly added
> > > > sequences from the 

MultiXactMemberFreezeThreshold can make autovacuum *less* aggressive

[Heikki Linnakangas]

The purpose of MultiXactMemberFreezeThreshold() is to make the effective 
autovacuum_multixact_freeze_max_age smaller, if the multixact members 
SLRU is approaching wraparound. Per comment there:



 * To prevent that, if more than a threshold portion of the members space is
 * used, we effectively reduce autovacuum_multixact_freeze_max_age and
 * to a value just less than the number of multixacts in use.  We hope that
 * this will quickly trigger autovacuuming on the table or tables with the
 * oldest relminmxid, thus allowing datminmxid values to advance and removing
 * some members.


However, the value that the function calculates can sometimes be 
*greater* than autovacuum_multixact_freeze_max_age. To get an overview 
of how it behaves, I wrote the attached stand-alone C program to test it 
with different inputs:


If members < MULTIXACT_MEMBER_SAFE_THRESHOLD, it just returns 
autovacuum_multixact_freeze_max_age, which is 200 million by default:


multixacts:100, members 10 ->  2
multixacts:100, members 20 ->  2
multixacts:100, members 21 ->  2

Above MULTIXACT_MEMBER_SAFE_THRESHOLD, the members-based calculated 
kicks in:


multixacts:100, members 22 -> 951091
multixacts:100, members 23 -> 857959
multixacts:100, members 25 -> 671694
multixacts:100, members 30 -> 206033
multixacts:100, members 31 -> 112901
multixacts:100, members 35 ->  0
multixacts:100, members 40 ->  0

However, if multixacts is also large the returned value is also quite large:

multixacts: 10, members 22 ->  951090335

That's larger than the default autovacuum_multixact_freeze_max_age! If 
you had set it to a lower non-default value, it's even worse.


I noticed this after I used pg_resetwal to reset next-multixid and 
next-mxoffset to a high value for testing purposes. Not sure how easy it 
is to reach that situation normally. In any case, I think the function 
should clamp the result to autovacuum_multixact_freeze_max_age, per 
attached.


--
Heikki Linnakangas
Neon (https://neon.tech)#include 
#include 

typedef uint32_t uint32;
typedef uint32 MultiXactOffset;

#define MaxMultiXactOffset	((MultiXactOffset) 0x)

#define MULTIXACT_MEMBER_SAFE_THRESHOLD		(MaxMultiXactOffset / 2)
#define MULTIXACT_MEMBER_DANGER_THRESHOLD	\
	(MaxMultiXactOffset - MaxMultiXactOffset / 4)

#define autovacuum_multixact_freeze_max_age 2

static int
test_MultiXactMemberFreezeThreshold(uint32 multixacts, MultiXactOffset members)
{
	uint32		victim_multixacts;
	double		fraction;

	/* If member space utilization is low, no special action is required. */
	if (members <= MULTIXACT_MEMBER_SAFE_THRESHOLD)
		return autovacuum_multixact_freeze_max_age;

	/*
	 * Compute a target for relminmxid advancement.  The number of multixacts
	 * we try to eliminate from the system is based on how far we are past
	 * MULTIXACT_MEMBER_SAFE_THRESHOLD.
	 */
	fraction = (double) (members - MULTIXACT_MEMBER_SAFE_THRESHOLD) /
		(MULTIXACT_MEMBER_DANGER_THRESHOLD - MULTIXACT_MEMBER_SAFE_THRESHOLD);
	victim_multixacts = multixacts * fraction;

	/* fraction could be > 1.0, but lowest possible freeze age is zero */
	if (victim_multixacts > multixacts)
		return 0;
	return multixacts - victim_multixacts;
}

static int
testit(uint32 multixacts, MultiXactOffset members)
{
  int result = test_MultiXactMemberFreezeThreshold(multixacts, members);

  printf("multixacts: %10u, members %10u -> %10d\n", multixacts, members, result);

  return result;
}


int main(int argc, char **argv)
{
  testit(   100UL, 10UL);
  testit(   100UL, 20UL);
  testit(   100UL, 21UL);
  testit(   100UL, 22UL);
  testit(   100UL, 23UL);
  testit(   100UL, 25UL);
  testit(   100UL, 30UL);
  testit(   100UL, 31UL);
  testit(   100UL, 35UL);
  testit(   100UL, 40UL);

  testit(10UL, 22UL);
}
From 25c18f6918c064e36865e8969fda8300a2864eca Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Wed, 12 Jun 2024 23:38:41 +0300
Subject: [PATCH 1/1] Clamp result of MultiXactMemberFreezeThreshold

The purpose of the function is to reduce the effective
autovacuum_multixact_freeze_max_age, to make multixid freezing more
aggressive, if the multixact members SLRU is approaching
wraparound. The returned value should thereore never be greater than
plain autovacuum_multixact_freeze_max_age is.

Discussion: xxx
---
 src/backend/access/transam/multixact.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 54c916e034..a4e732a3f5 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -2932,6 +2932,7 @@ 

Re: Add support to TLS 1.3 cipher suites and curves lists

[Daniel Gustafsson]

> On 13 Jun 2024, at 09:07, Erica Zhang  wrote:

> How can I achieve the value for TLS1.3? Do you mean I can set the 
> Ciphersuites in openssl.conf, then Postgres will pick up and use this value 
> accordingly?

Yes, you should be able to restrict the ciphersuites for TLSv1.3 with
openssl.conf on your system.

--
Daniel Gustafsson

Re: Track the amount of time waiting due to cost_delay

[Bertrand Drouvot]

Hi,

On Tue, Jun 11, 2024 at 08:26:23AM +, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Jun 11, 2024 at 04:07:05PM +0900, Masahiko Sawada wrote:
> 
> > Thank you for the proposal and the patch. I understand the motivation
> > of this patch.
> 
> Thanks for looking at it!
> 
> > Beside the point Nathan mentioned, I'm slightly worried
> > that massive parallel messages could be sent to the leader process
> > when the cost_limit value is low.
> 
> I see, I can/will do some testing in this area and share the numbers.

Here is the result of the test. It has been launched several times and it
produced the same (surprising result) each time.

== Context 

The testing has been done with this relation (large_tbl) and its indexes:

postgres=# SELECT n.nspname, c.relname, count(*) AS buffers
 FROM pg_buffercache b JOIN pg_class c
 ON b.relfilenode = pg_relation_filenode(c.oid) AND
b.reldatabase IN (0, (SELECT oid FROM pg_database
  WHERE datname = current_database()))
 JOIN pg_namespace n ON n.oid = c.relnamespace
 GROUP BY n.nspname, c.relname
 ORDER BY 3 DESC
 LIMIT 22;

 nspname |  relname   | buffers
-++-
 public  | large_tbl  |  80
 public  | large_tbl_filler13 |  125000
 public  | large_tbl_filler6  |  125000
 public  | large_tbl_filler5  |  125000
 public  | large_tbl_filler3  |  125000
 public  | large_tbl_filler15 |  125000
 public  | large_tbl_filler4  |  125000
 public  | large_tbl_filler20 |  125000
 public  | large_tbl_filler18 |  125000
 public  | large_tbl_filler14 |  125000
 public  | large_tbl_filler8  |  125000
 public  | large_tbl_filler11 |  125000
 public  | large_tbl_filler19 |  125000
 public  | large_tbl_filler7  |  125000
 public  | large_tbl_filler1  |  125000
 public  | large_tbl_filler12 |  125000
 public  | large_tbl_filler9  |  125000
 public  | large_tbl_filler17 |  125000
 public  | large_tbl_filler16 |  125000
 public  | large_tbl_filler10 |  125000
 public  | large_tbl_filler2  |  125000
 public  | large_tbl_pkey |5486
(22 rows)

All of them completly fit in memory (to avoid I/O read latency during the 
vacuum).

The config, outside of default is:

max_wal_size = 4GB
shared_buffers = 30GB
vacuum_cost_delay = 1
autovacuum_vacuum_cost_delay = 1
max_parallel_maintenance_workers = 8
max_parallel_workers = 10
vacuum_cost_limit = 10
autovacuum_vacuum_cost_limit = 10

My system is not overloaded, has enough resources to run this test and only this
test is running.

== Results 

== With v2 (attached) applied on master

postgres=# VACUUM (PARALLEL 8) large_tbl;
VACUUM
Time: 1146873.016 ms (19:06.873)

The duration is splitted that way:

Vacuum phase: cumulative time (cumulative time delayed)
===
scanning heap: 00:08:16.414628 (time_delayed: 444370)
vacuuming indexes: 00:14:55.314699 (time_delayed: 2545293)
vacuuming heap: 00:19:06.814617 (time_delayed: 2767540)

I sampled active sessions from pg_stat_activity (one second interval), here is
the summary during the vacuuming indexes phase (ordered by count):

 leader_pid |  pid   |   wait_event   | count
+++---
 452996 | 453225 | VacuumDelay|   366
 452996 | 453223 | VacuumDelay|   363
 452996 | 453226 | VacuumDelay|   362
 452996 | 453224 | VacuumDelay|   361
 452996 | 453222 | VacuumDelay|   359
 452996 | 453221 | VacuumDelay|   359
| 452996 | VacuumDelay|   331
| 452996 | CPU|30
 452996 | 453224 | WALWriteLock   |23
 452996 | 453222 | WALWriteLock   |20
 452996 | 453226 | WALWriteLock   |20
 452996 | 453221 | WALWriteLock   |19
| 452996 | WalSync|18
 452996 | 453225 | WALWriteLock   |18
 452996 | 453223 | WALWriteLock   |16
| 452996 | WALWriteLock   |15
 452996 | 453221 | CPU|14
 452996 | 453222 | CPU|14
 452996 | 453223 | CPU|12
 452996 | 453224 | CPU|10
 452996 | 453226 | CPU|10
 452996 | 453225 | CPU| 8
 452996 | 453223 | WalSync| 4
 452996 | 453221 | WalSync| 2
 452996 | 453226 | WalWrite   | 2
 452996 | 453221 | WalWrite   | 1
| 452996 | ParallelFinish | 1
 452996 | 453224 | WalSync| 1
 452996 | 453225 | WalSync| 1
 452996 | 453222 | WalWrite   | 1
 452996 | 453225 | WalWrite   | 1
 452996 | 453222 | WalSync| 1
 452996 | 453226 | WalSync| 1


== On master (v2 

Re: CI and test improvements

[Justin Pryzby]

On Thu, Jun 13, 2024 at 02:38:46PM +0300, Nazir Bilal Yavuz wrote:
> On Wed, 12 Jun 2024 at 16:10, Justin Pryzby  wrote:
> > On Fri, Jun 24, 2022 at 08:38:50AM +1200, Thomas Munro wrote:
> > > > I've also sent some patches to Thomas for cfbot to help progress some 
> > > > of these
> > > > patches (code coverage and documentation upload as artifacts).
> > > > https://github.com/justinpryzby/cfbot/commits/master
> > >
> > > Thanks, sorry for lack of action, will get to these soon.
> >
> > On Tue, Feb 13, 2024 at 01:10:21PM -0600, Justin Pryzby wrote:
> > > I sent various patches to cfbot but haven't heard back.
> >
> > > https://www.postgresql.org/message-id/flat/20220409021853.gp24...@telsasoft.com
> > > https://www.postgresql.org/message-id/flat/20220623193125.gb22...@telsasoft.com
> > > https://github.com/justinpryzby/cfbot/commits/master
> > > https://github.com/macdice/cfbot/pulls
> >
> > @Thomas: ping
> >
> > I reintroduced the patch for ccache/windows -- v4.10 supports PCH, which
> > can make the builds 2x faster.
> 
> I applied 0001 and 0002 to see ccache support on Windows but the build
> step failed with: 'ccache: error: No stats log has been configured'.
> Perhaps you forgot to add 'CCACHE_STATSLOG: $CCACHE_DIR.stats.log' to
> 0002?

Something like that - I put the line back.  I don't know if statslog
should be included in the patch, but it's useful for demonstrating that
it's working.

ccache should be installed in the image rather than re-installed on each
invocation.

-- 
Justin
>From ad03da2cfa3ecf23334f8b31f2e4529ba3094512 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 25 May 2022 21:53:22 -0500
Subject: [PATCH 1/6] cirrus/windows: add compiler_warnings_script

The goal is to fail due to warnings only after running tests.
(At least historically, it's too slow to run a separate windows VM to
compile with -Werror.)

https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de

I'm not sure how to write this test in windows shell; it's also easy to
write something that doesn't work in posix sh, since windows shell is
interpretting && and ||...

See also:
8a1ce5e54f6d144e4f8e19af7c767b026ee0c956
https://cirrus-ci.com/task/6241060062494720
https://cirrus-ci.com/task/6496366607204352

20221104161934.gb16...@telsasoft.com
20221031223744.gt16...@telsasoft.com
20221104161934.gb16...@telsasoft.com
20221123054329.gg11...@telsasoft.com
20230224002029.gq1...@telsasoft.com

ci-os-only: windows
---
 .cirrus.tasks.yml | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 33646faeadf..5a2b64f64c2 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -566,12 +566,21 @@ task:
 
   build_script: |
 vcvarsall x64
-ninja -C build
+ninja -C build |tee build.txt
+REM Since pipes lose the exit status of the preceding command, rerun the compilation
+REM without the pipe, exiting now if it fails, to avoid trying to run checks
+ninja -C build > nul
 
   check_world_script: |
 vcvarsall x64
 meson test %MTEST_ARGS% --num-processes %TEST_JOBS%
 
+  # This should be last, so check_world is run even if there are warnings
+  always:
+compiler_warnings_script:
+  # this avoids using metachars which would be interpretted by the windows shell
+  - sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0'
+
   on_failure:
 <<: *on_failure_meson
 crashlog_artifacts:
-- 
2.42.0

>From 31629f2f413ceb5f2704ac71e24d0d8e82cc26ab Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 25 May 2022 22:05:13 -0500
Subject: [PATCH 2/6] WIP: cirrus/windows: ccache

ccache v4.10 supports PCH, so this now seems viable.
(ccache 4.7 added support for depend mode with MSVC).

https://www.postgresql.org/message-id/flat/20220522232606.GZ19626%40telsasoft.com

ccache should be installed in the VM rather than with choco in each
invocation.

https://cirrus-ci.com/task/5213936495099904 build 29sec

, linux
linux-meson
ci-os-only: windows, windows-msvc
---
 .cirrus.tasks.yml | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 5a2b64f64c2..d8c2a396a20 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -536,6 +536,12 @@ task:
   env:
 TEST_JOBS: 8 # wild guess, data based value welcome
 
+CCACHE_DIR: $CIRRUS_WORKING_DIR/.ccache
+CCACHE_MAXSIZE: "500M"
+CCACHE_SLOPPINESS: pch_defines,time_macros
+CCACHE_DEPEND: 1
+CCACHE_STATSLOG: $CCACHE_DIR.stats.log
+
 # Cirrus defaults to SetErrorMode(SEM_NOGPFAULTERRORBOX | ...). That
 # prevents crash reporting from working unless binaries do SetErrorMode()
 # themselves. Furthermore, it appears that either python or, more likely,
@@ -550,8 +556,11 @@ task:
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*windows.*'
 
-  

Re: Improve the granularity of PQsocketPoll's timeout parameter?

[Ranier Vilela]

Em qua., 12 de jun. de 2024 às 16:45, Tom Lane  escreveu:

> I wrote:
> > v3 attached uses pg_usec_time_t, and fixes one brown-paper-bag
> > bug the cfbot noticed in v2.
>
> Oh, I just remembered that there's a different bit of
> pqConnectDBComplete that we could simplify now:
>
> if (timeout > 0)
> {
> /*
>  * Rounding could cause connection to fail unexpectedly
> quickly;
>  * to prevent possibly waiting hardly-at-all, insist on at
> least
>  * two seconds.
>  */
> if (timeout < 2)
> timeout = 2;
> }
> else/* negative means 0 */
> timeout = 0;
>
> With this infrastructure, there's no longer any need to discriminate
> against timeout == 1 second, so we might as well reduce this to
>
> if (timeout < 0)
> timeout = 0;
>
> as it's done elsewhere.
>
I'm unsure if the documentation matches the code?
" connect_timeout #


Maximum time to wait while connecting, in seconds (write as a decimal
integer, e.g., 10). Zero, negative, or not specified means wait
indefinitely. The minimum allowed timeout is 2 seconds, therefore a value
of 1 is interpreted as 2. This timeout applies separately to each host name
or IP address. For example, if you specify two hosts and connect_timeout is
5, each host will time out if no connection is made within 5 seconds, so
the total time spent waiting for a connection might be up to 10 seconds.
"
The comments says that timeout = 0, means *Timeout is immediate (no
blocking)*

Does the word "indefinitely" mean infinite?
If yes, connect_timeout = -1, mean infinite?

best regards,
Ranier Vilela

Re: CI and test improvements

[Nazir Bilal Yavuz]

Hi,

Thanks for working on this!

On Wed, 12 Jun 2024 at 16:10, Justin Pryzby  wrote:
>
> On Fri, Jun 24, 2022 at 08:38:50AM +1200, Thomas Munro wrote:
> > > I've also sent some patches to Thomas for cfbot to help progress some of 
> > > these
> > > patches (code coverage and documentation upload as artifacts).
> > > https://github.com/justinpryzby/cfbot/commits/master
> >
> > Thanks, sorry for lack of action, will get to these soon.
>
> On Tue, Feb 13, 2024 at 01:10:21PM -0600, Justin Pryzby wrote:
> > I sent various patches to cfbot but haven't heard back.
>
> > https://www.postgresql.org/message-id/flat/20220409021853.gp24...@telsasoft.com
> > https://www.postgresql.org/message-id/flat/20220623193125.gb22...@telsasoft.com
> > https://github.com/justinpryzby/cfbot/commits/master
> > https://github.com/macdice/cfbot/pulls
>
> @Thomas: ping
>
> I reintroduced the patch for ccache/windows -- v4.10 supports PCH, which
> can make the builds 2x faster.

I applied 0001 and 0002 to see ccache support on Windows but the build
step failed with: 'ccache: error: No stats log has been configured'.
Perhaps you forgot to add 'CCACHE_STATSLOG: $CCACHE_DIR.stats.log' to
0002? After adding that line, CI finished successfully. And, I confirm
that the build step takes ~30 seconds now; it was ~90 seconds before
that.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Conflict Detection and Resolution

[Amit Kapila]

On Thu, Jun 13, 2024 at 11:41 AM Masahiko Sawada  wrote:
>
> On Wed, Jun 5, 2024 at 3:32 PM Zhijie Hou (Fujitsu)
>  wrote:
> >
> > Hi,
> >
> > This time at PGconf.dev[1], we had some discussions regarding this
> > project. The proposed approach is to split the work into two main
> > components. The first part focuses on conflict detection, which aims to
> > identify and report conflicts in logical replication. This feature will
> > enable users to monitor the unexpected conflicts that may occur. The
> > second part involves the actual conflict resolution. Here, we will provide
> > built-in resolutions for each conflict and allow user to choose which
> > resolution will be used for which conflict(as described in the initial
> > email of this thread).
>
> I agree with this direction that we focus on conflict detection (and
> logging) first and then develop conflict resolution on top of that.
>
> >
> > Of course, we are open to alternative ideas and suggestions, and the
> > strategy above can be changed based on ongoing discussions and feedback
> > received.
> >
> > Here is the patch of the first part work, which adds a new parameter
> > detect_conflict for CREATE and ALTER subscription commands. This new
> > parameter will decide if subscription will go for conflict detection. By
> > default, conflict detection will be off for a subscription.
> >
> > When conflict detection is enabled, additional logging is triggered in the
> > following conflict scenarios:
> >
> > * updating a row that was previously modified by another origin.
> > * The tuple to be updated is not found.
> > * The tuple to be deleted is not found.
> >
> > While there exist other conflict types in logical replication, such as an
> > incoming insert conflicting with an existing row due to a primary key or
> > unique index, these cases already result in constraint violation errors.
>
> What does detect_conflict being true actually mean to users? I
> understand that detect_conflict being true could introduce some
> overhead to detect conflicts. But in terms of conflict detection, even
> if detect_confict is false, we detect some conflicts such as
> concurrent inserts with the same key. Once we introduce the complete
> conflict detection feature, I'm not sure there is a case where a user
> wants to detect only some particular types of conflict.
>

You are right that users would wish to detect the conflicts and
probably the extra effort would only be in the 'update_differ' case
where we need to consult committs module and that we will only do when
'track_commit_timestamp' is true. BTW, I think for Inserts with
primary/unique key violation, we should catch the ERROR and log it. If
we want to log the conflicts in a separate table then do we want to do
that in the catch block after getting pk violation or do an extra scan
before 'INSERT' to find the conflict? I think logging would need extra
cost especially if we want to LOG it in some table as you are
suggesting below that may need some option.

> > Therefore, additional conflict detection for these cases is currently
> > omitted to minimize potential overhead. However, the pre-detection for
> > conflict in these error cases is still essential to support automatic
> > conflict resolution in the future.
>
> I feel that we should log all types of conflict in an uniform way. For
> example, with detect_conflict being true, the update_differ conflict
> is reported as "conflict %s detected on relation "%s"", whereas
> concurrent inserts with the same key is reported as "duplicate key
> value violates unique constraint "%s"", which could confuse users.
> Ideally, I think that we log such conflict detection details (table
> name, column name, conflict type, etc) to somewhere (e.g. a table or
> server logs) so that the users can resolve them manually.
>

It is good to think if there is a value in providing in
pg_conflicts_history kind of table which will have details of
conflicts that occurred and then we can extend it to have resolutions.
I feel we can anyway LOG the conflicts by default. Updating a separate
table with conflicts should be done by default or with a knob is a
point to consider.

-- 
With Regards,
Amit Kapila.

Re: RFC: adding pytest as a supported test framework

[Daniel Gustafsson]

> On 13 Jun 2024, at 00:34, Melanie Plageman  wrote:

> FWIW, I felt a lot of pain trying to write recovery TAP tests with
> IPC::Run's pumping functionality. It was especially painful (as
> someone who knows even less Perl than the "street fighting Perl"
> Thomas Munro has described having) before the additional test
> infrastructure was added in BackgroudPsql.pm last year.

A key aspect of this, which isn't specific to Perl or our use of it, is that
this was done in backbranches which doesn't have the (recently) much improved
BackgroundPsql.pm.  The quality of our tools and the ease of use they provide
is directly related to the investment we make into continuously improving our
testharness.  Regardless of which toolset we adopt, if we don't make this
investment (taking learnings from the past years and toolsets into account)
we're bound to repeat this thread in a few years advocating for toolset X+1.

--
Daniel Gustafsson

Re: Show WAL write and fsync stats in pg_stat_io

[Nazir Bilal Yavuz]

Hi,

Thank you for looking into this!

On Sun, 9 Jun 2024 at 18:05, Nitin Jadhav  wrote:
>
> > If possible, let's have all the I/O stats (even for WAL) in
> > pg_stat_io. Can't we show the WAL data we get from buffers in the hits
> > column and then have read_bytes or something like that to know the
> > amount of data read?
>
> The ‘hits’ column in ‘pg_stat_io’ is a vital indicator for adjusting a
> database. It signifies the count of cache hits, or in other words, the
> instances where data was located in the ‘shared_buffers’. As a result,
> keeping an eye on the ‘hits’ column in ‘pg_stat_io’ can offer useful
> knowledge about the buffer cache’s efficiency and assist users in
> making educated choices when fine-tuning their database. However, if
> we include the hit count of WAL buffers in this, it may lead to
> misleading interpretations for database tuning. If there’s something
> I’ve overlooked that’s already been discussed, please feel free to
> correct me.

I think counting them as a hit makes sense. We read data from WAL
buffers instead of reading them from disk. And, WAL buffers are stored
in shared memory so I believe they can be counted as hits in the
shared buffers. Could you please explain how this change can 'lead to
misleading interpretations for database tuning' a bit more?

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Optimize numeric.c mul_var() using the Karatsuba algorithm

[Joel Jacobson]

On Tue, Jun 11, 2024, at 19:16, Aaron Altman wrote:
> Hi Joel, thanks for posting this.  Although I have only a cursory 
> familiarity with fast multiplication algorithms, I'd like to try and 
> give it a review.  To start with, can you help me understand the choice 
> of this algorithm versus others like Toom?  If this looks correct on a 
> closer view I'll propose it for inclusion. Along the way though I'd like 
> to have it explicitly called out whether this is superior in general to 
> other choices, better for more realistic use cases, simpler, clearer to 
> license or something similar.  It would be nice for future dicussions to 
> have some context around whether it would make sense to have conditions 
> to choose other algorithms as well, or if this one is generally the best 
> for what Postgres users are usually doing.
>
> Continuing with code review in any case.  Interested to hear more.

Hi Aaron, thanks for looking at this!

The choice of best algorithm depends on the factor sizes.

The larger factor sizes, the more complicated algorithm can be "afforded".

List of fast multiplication algorithms,
ordered by factor sizes they are suitable for:

- Long multiplication, aka "schoolbook" multiplication.
- Karatsuba
- Toom-3
- Schönhage–Strassen algorithm (Fast Fourier Transform)

The Toom-3 algorithm can be modified to split the smaller and larger factors
into different number of parts. The notation used at Wikipedia is e.g. Toom-2.5
which I think means splitting the larger into three parts, and the smaller into
two parts, while GMP uses Toom32 to mean the same thing.
Personally, I think GMPs notation is easier to understand as the number of parts
can be directly derived from the name.

I experimented with implementing Toom-3 as well, but there was only a marginal
win, at very large factor sizes, and since numeric's max ndigits
(number of base-digits) is capped at 32768, I didn't think it was worth it,
since it adds quite a lot of complexity.

The Karatsuba algorithm is the next step in the hierarchy of fast multiplication
algorithms, and all other bigint libs I've looked at implement Karatsuba,
even if they also implement Toom-3, since Karatsuba is faster than Toom-3 for
sufficiently small factors, but that are at the same time sufficiently large for
Karatsuba to be faster than schoolbook.

I was initially surprised by the quite large threshold, where Karatsuba started
to be a win over schoolbook.

I think the explanation why mul_var() stays fast up to quite remarkably high
factor sizes, could be a combination of several things, such as:

- mul_var() is already heavily optimized, with clever tricks,
  such as deferred carry propagation.

- numeric uses NBASE=1, while other bigint libs usually use a power of two.

In the Karatsuba implementation, I tried to keep the KARATSUBA_CONDITION()
quite simple, but it's way more complex than what most bigint libs use,
that usually just check if the smaller factor is smaller than some threshold,
and if so, use schoolbook. For instance, this is what Rust's num-bigint does:

if x.len() <= 32 {
// Long multiplication
} else if x.len() * 2 <= y.len() {
// Half-Karatsuba, for factors with significant length disparity
} else if x.len() <= 256 {
// Karatsuba multiplication
} else {
// Toom-3 multiplication
}

Source: 
https://github.com/rust-num/num-bigint/blob/master/src/biguint/multiplication.rs#L101

Side note: When working on Karatsuba in mul_var(), I looked at some other bigint
implementations, to try to understand their threshold functions.
I noticed that Rust's num-bigint didn't optimise for factors with significant
length disparity, so I contributed a patch based on my "Half-Karatsuba" idea,
that I got when working with mul_var(), which has now been merged:
https://github.com/rust-num/num-bigint/commit/06b61c8138ad8a9959ac54d9773d0a9ebe25b346

In mul_var(), if we don't like the complexity of KARATSUBA_CONDITION(),
we could go for a more traditional threshold approach, i.e. just checking
the smaller factor size. However, I believe that would be at the expense
of missing out of some performance gains.

I've tried quite hard to find the best KARATSUBA_CONDITION(), but I found it to
be a really hard problem, the differences between different CPU architectures,
in combination with wanting a simple expression, means there is no obvious
perfect threshold function, there will always be a trade-off.

I eventually stopped trying to improve it, and just settled on the version in
the patch, and thought that I'll leave it up to the community to give feedback
on what complexity for the threshold function is motivated. If we absolutely
just want to check the smallest factor size, like Rust, then it's super simple,
then the threshold can easily be found just by testing different values.
It's when both factor sizes are input to the threshold function that makes it
complicated.

/Joel

Re: Show WAL write and fsync stats in pg_stat_io

[Nazir Bilal Yavuz]

Hi,

Thank you for looking into this! And, sorry for the late answer.

On Mon, 13 May 2024 at 17:12, Bharath Rupireddy
 wrote:
>
> On Fri, Apr 19, 2024 at 1:32 PM Nazir Bilal Yavuz  wrote:
> >
> > > I wanted to inform you that the 73f0a13266 commit changed all WALRead
> > > calls to read variable bytes, only the WAL receiver was reading
> > > variable bytes before.
> >
> > I want to start working on this again if possible. I will try to
> > summarize the current status:
>
> Thanks for working on this.
>
> > * With the 73f0a13266 commit, the WALRead() function started to read
> > variable bytes in every case. Before, only the WAL receiver was
> > reading variable bytes.
> >
> > * With the 91f2cae7a4 commit, WALReadFromBuffers() is merged. We were
> > discussing what we have to do when this is merged. It is decided that
> > WALReadFromBuffers() does not call pgstat_report_wait_start() because
> > this function does not perform any IO [1]. We may follow the same
> > logic by not including these to pg_stat_io?
>
> Right. WALReadFromBuffers doesn't do any I/O.
>
> Whoever reads WAL from disk (backends, walsenders, recovery process)
> using pg_pread (XLogPageRead, WALRead) needs to be tracked in
> pg_stat_io or some other view. If it were to be in pg_stat_io,
> although we may not be able to distinguish WAL read stats at a backend
> level (like how many times/bytes a walsender or recovery process or a
> backend read WAL from disk), but it can help understand overall impact
> of WAL read I/O at a cluster level. With this approach, the WAL I/O
> stats are divided up - WAL read I/O and write I/O stats are in
> pg_stat_io and pg_stat_wal respectively.
>
> This makes me think if we need to add WAL read I/O stats also to
> pg_stat_wal. Then, we can also add WALReadFromBuffers stats
> hits/misses there. With this approach, pg_stat_wal can be a one-stop
> view for all the WAL related stats. If needed, we can join info from
> pg_stat_wal to pg_stat_io in system_views.sql so that the I/O stats
> are emitted to the end-user via pg_stat_io.

I agree that the ultimate goal is seeing WAL I/O stats from one place.
There is a reply to this from Amit:

On Tue, 28 May 2024 at 03:48, Amit Kapila  wrote:
>
> If possible, let's have all the I/O stats (even for WAL) in
> pg_stat_io. Can't we show the WAL data we get from buffers in the hits
> column and then have read_bytes or something like that to know the
> amount of data read?

I think it is better to have all the I/O stats in pg_stat_io like Amit
said. And, it makes sense to me to show 'WAL data we get from buffers'
in the hits column. Since, basically instead of doing I/O from disk;
we get data directly from WAL buffers. I think that fits the
explanation of the hits column in pg_stat_io, which is 'The number of
times a desired block was found in a shared buffer.' [1].

> > * With the b5a9b18cd0 commit, streaming I/O is merged but AFAIK this
> > does not block anything related to putting WAL stats in pg_stat_io.
> >
> > If I am not missing any new changes, the only problem is reading
> > variable bytes now. We have discussed a couple of solutions:
> >
> > 1- Change op_bytes to something like -1, 0, 1, NULL etc. and document
> > that this means some variable byte I/O is happening.
> >
> > I kind of dislike this solution because if the *only* read I/O is
> > happening in variable bytes, it will look like write and extend I/Os
> > are happening in variable bytes as well. As a solution, it could be
> > documented that only read I/Os could happen in variable bytes for now.
>
> Yes, read I/O for relation and WAL can happen in variable bytes. I
> think this idea seems reasonable and simple yet useful to know the
> cluster-wide read I/O.

I agree.

> However, another point here is how the total number of bytes read is
> represented with existing pg_stat_io columns 'reads' and 'op_bytes'.
> It is known now with 'reads' * 'op_bytes', but with variable bytes,
> how is read bytes calculated? Maybe add new columns
> read_bytes/write_bytes?
>
> > 2- Use op_bytes_[read | write | extend] columns instead of one
> > op_bytes column, also use the first solution.
> >
> > This can solve the first solution's weakness but it introduces two
> > more columns. This is more future proof compared to the first solution
> > if there is a chance that some variable I/O could happen in write and
> > extend calls as well in the future.
>
> -1 as more columns impact the readability and usability.

I did not understand the overall difference between what you suggested
(adding read_bytes/write_bytes columns) and my suggestion (adding
op_bytes_[read | write | extend] columns). They both introduce new
columns. Could you please explain what you suggested in more detail?

> > 3- Create a new pg_stat_io_wal view to put WAL I/Os here instead of 
> > pg_stat_io.
> >
> > pg_stat_io could remain untouchable and we will have flexibility to
> > edit this new view as much as we want. But the original aim of the
> > pg_stat_io 

Re: Proposal to add page headers to SLRU pages

[Li, Yong]

> On Jun 10, 2024, at 16:01, Michael Paquier  wrote:
> 
> External Email
> 
> From: Michael Paquier 
> Subject: Re: Proposal to add page headers to SLRU pages
> Date: June 10, 2024 at 16:01:50 GMT+8
> To: Bertrand Drouvot 
> Cc: "Li, Yong" , Jeff Davis , Aleksander 
> Alekseev , PostgreSQL Hackers 
> , "Bagga, Rishu" , 
> Robert Haas , Andrey Borodin , 
> "Shyrabokau, Anton" 
> 
> 
> On Mon, Jun 10, 2024 at 07:19:56AM +, Bertrand Drouvot wrote:
>> On Tue, Mar 19, 2024 at 06:48:33AM +, Li, Yong wrote:
>>> Unfortunately, the test requires a setup of two different versions of PG. I 
>>> am not
>>> aware of an existing test infrastructure which can run automated tests 
>>> using two
>>> PGs. I did manually verify the output of pg_upgrade.
>> 
>> I think there is something in t/002_pg_upgrade.pl (see 
>> src/bin/pg_upgrade/TESTING),
>> that could be used to run automated tests using an old and a current version.
> 
> Cluster.pm relies on install_path for stuff, where it is possible to
> create tests with multiple nodes pointing to different installation
> paths.  This allows mixing nodes with different build options, or just
> different major versions like pg_upgrade's perl tests.
> —
> Michael
> 
> 

Thanks for pointing this out. Here is what I have tried:
1. Manually build and install PostgreSQL from the latest source code.
2. Following the instructions from src/bin/pg_upgrade to manually dump the 
regression database.
3. Apply the patch to the latest code, and build from the source.
4. Run “make check” by following the instructions from src/bin/pg_upgrade and 
setting up the olddump and oldinstall to point to the “old” installation used 
in step 2.

All tests pass.


Yong