Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Tom Lane]

Amit Kapila  writes:
> Right, but as Tom pointed it is still better to change this. However,
> I am not sure if we should backpatch this to PG15 as this won't lead
> to any incorrect behavior.

If that code only exists in HEAD and v15 then I'd backpatch.
It's a very low-risk change and it might avoid merge problems
for future backpatches.

regards, tom lane

Re: including pid's for `There are XX other sessions using the database`

[Euler Taveira]

On Fri, Aug 19, 2022, at 2:10 PM, Zhihong Yu wrote:
> I want to poll the community on whether including proc->pid's in the error 
> message would be useful for troubleshooting.
Such message is only useful for a parameter into a pg_stat_activity query. You
don't need the PID list if you already have the most important information:
database name. I don't think revealing the current session PIDs from the
database you want to drop will buy you anything. It could be a long list and it
does not help you to solve the issue: why wasn't that database removed?
 
Besides that, if you know that there is a possibility that a connection is
open, you can always use the FORCE option. The old/other alternative is to use
a query like
 
   SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = 'foo';
 
(possibly combined with a REVOKE CONNECT or pg_hba.conf modification) before
executing DROP DATABASE.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Amit Kapila]

On Fri, Aug 19, 2022 at 7:45 PM Ranier Vilela  wrote:
>
> Em sex., 19 de ago. de 2022 às 10:28, Tom Lane  escreveu:
>>
>> Ranier Vilela  writes:
>> > At function parallel_vacuum_process_all_indexes there is
>> > a typo with a logical connector.
>> > I think that correct is &&, because both of the operators are
>> > bool types [1].
>> > As a result, parallel vacuum workers can be incorrectly enabled.
>>
>> Since they're bools, the C spec requires them to promote to integer
>> 0 or 1, therefore the & operator will yield the desired result.
>> So there's not going to be any incorrect behavior.
>
>
> So, my assumption is incorrect.
>

Right, but as Tom pointed it is still better to change this. However,
I am not sure if we should backpatch this to PG15 as this won't lead
to any incorrect behavior.

-- 
With Regards,
Amit Kapila.

sslinfo extension - add notbefore and notafter timestamps

[Cary Huang]

Hello



I noticed that sslinfo extension does not have functions to return current 
client certificate's notbefore and notafter timestamps which are also quite 
important attributes in a X509 certificate. The attached patch adds 2 functions 
to get notbefore and notafter timestamps from the currently connected client 
certificate.



thank you!







Cary Huang

-

HighGo Software Inc. (Canada)

mailto:cary.hu...@highgo.ca

http://www.highgo.ca

v1-0001-sslinfo-add-notbefore-and-notafter-timestamps.patch
Description: Binary data

Re: use ARM intrinsics in pg_lfind32() where available

[Nathan Bossart]

On Fri, Aug 19, 2022 at 02:26:02PM -0700, Andres Freund wrote:
> Are you sure there's not an appropriate define for us to use here instead of a
> configure test? E.g.
> 
> echo|cc -dM -P -E -|grep -iE 'arm|aarch'
> ...
> #define __AARCH64_SIMD__ 1
> ...
> #define __ARM_NEON 1
> #define __ARM_NEON_FP 0xE
> #define __ARM_NEON__ 1
> ..
> 
> I strikes me as non-scalable to explicitly test all the simd instructions we'd
> use.

Thanks for the pointer.  GCC, Clang, and the Arm compiler all seem to
define __ARM_NEON, so here is a patch that uses that instead.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 5f068010d30c2a92003e43fa655eab5db8ab7ec2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 19 Aug 2022 15:23:09 -0700
Subject: [PATCH v2 1/1] Use ARM Advanced SIMD intrinsic functions in
 pg_lfind32().

Use ARM Advanced SIMD intrinsic functions to speed up the search,
where available.  Otherwise, use a simple 'for' loop as before.  As
with b6ef167, this speeds up XidInMVCCSnapshot(), but any uses of
pg_lfind32() will also benefit.

Author: Nathan Bossart
---
 src/include/port/pg_lfind.h | 34 ++
 src/include/port/simd.h |  8 
 2 files changed, 42 insertions(+)

diff --git a/src/include/port/pg_lfind.h b/src/include/port/pg_lfind.h
index fb125977b2..41a371681d 100644
--- a/src/include/port/pg_lfind.h
+++ b/src/include/port/pg_lfind.h
@@ -82,6 +82,40 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem)
 	}
 #endif			/* USE_SSE2 */
 
+#ifdef __ARM_NEON
+	/*
+	 * A 16-byte register only has four 4-byte lanes. For better
+	 * instruction-level parallelism, each loop iteration operates on a block
+	 * of four registers.
+	 */
+	const		uint32x4_t keys = vdupq_n_u32(key); /* load 4 copies of key */
+	uint32		iterations = nelem & ~0xF;  /* round down to multiple of 16 */
+
+	for (i = 0; i < iterations; i += 16)
+	{
+		/* load the next block into 4 registers holding 4 values each */
+		const		uint32x4_t vals1 = vld1q_u32((const uint32 *) & base[i]);
+		const		uint32x4_t vals2 = vld1q_u32((const uint32 *) & base[i + 4]);
+		const		uint32x4_t vals3 = vld1q_u32((const uint32 *) & base[i + 8]);
+		const		uint32x4_t vals4 = vld1q_u32((const uint32 *) & base[i + 12]);
+
+		/* compare each value to the key */
+		const		uint32x4_t result1 = vceqq_u32(keys, vals1);
+		const		uint32x4_t result2 = vceqq_u32(keys, vals2);
+		const		uint32x4_t result3 = vceqq_u32(keys, vals3);
+		const		uint32x4_t result4 = vceqq_u32(keys, vals4);
+
+		/* combine the results into a single variable */
+		const		uint32x4_t tmp1 = vorrq_u32(result1, result2);
+		const		uint32x4_t tmp2 = vorrq_u32(result3, result4);
+		const		uint32x4_t result = vorrq_u32(tmp1, tmp2);
+
+		/* see if there was a match */
+		if (vmaxvq_u32(result) != 0)
+			return true;
+	}
+#endif			/* __ARM_NEON */
+
 	/* Process the remaining elements one at a time. */
 	for (; i < nelem; i++)
 	{
diff --git a/src/include/port/simd.h b/src/include/port/simd.h
index a571e79f57..67df6ef439 100644
--- a/src/include/port/simd.h
+++ b/src/include/port/simd.h
@@ -27,4 +27,12 @@
 #define USE_SSE2
 #endif
 
+/*
+ * Include arm_neon.h if the compiler is targeting an architecture that
+ * supports ARM Advanced SIMD (Neon) intrinsics.
+ */
+#ifdef __ARM_NEON
+#include 
+#endif
+
 #endif			/* SIMD_H */
-- 
2.25.1

Re: Avoid unecessary MemSet call (src/backend/utils/cache/relcache.c)

[David Zhang]

Hi Ranier,

Following the comment in commit 9fd45870c1436b477264c0c82eb195df52bc0919,

    (The same could be done with appropriate memset() calls, but this
    patch is part of an effort to phase out MemSet(), so it doesn't touch
    memset() calls.)

Should these obviously possible replacement of the standard library 
function "memset" be considered as well? For example, something like the 
attached one which is focusing on the pageinspect extension only.



Best regards,

David

On 2022-08-01 10:08 a.m., Ranier Vilela wrote:
Em sáb., 16 de jul. de 2022 às 16:54, Ranier Vilela 
 escreveu:




Em sáb, 16 de jul de 2022 2:58 AM, Peter Eisentraut
 escreveu:

On 11.07.22 21:06, Ranier Vilela wrote:
> Em qui., 7 de jul. de 2022 às 14:01, Ranier Vilela
 > escreveu:
>
>     Attached the v1 of your patch.
>     I think that all is safe to switch MemSet by {0}.
>
> Here the rebased patch v2, against latest head.

I have committed my patch with Álvaro's comments addressed

I see.
It's annoing that old compiler (gcc 4.7.2) don't handle this style.


Your patch appears to add in changes that are either arguably
out of
scope or would need further review (e.g., changing memset()
calls,
changing the scope of some variables, changing places that
need to worry
about padding bits).  Please submit separate patches for
those, and we
can continue the analysis.

Sure.

Hi, sorry for the delay.
Like how 
https://github.com/postgres/postgres/commit/9fd45870c1436b477264c0c82eb195df52bc0919

New attempt to remove more MemSet calls, that are safe.

Attached v3 patch.

regards,
Ranier Vilela
From 25bd8af7acfd6bf2e23cdfac93828d810cfbb5b4 Mon Sep 17 00:00:00 2001
From: David Zhang 
Date: Fri, 19 Aug 2022 14:43:01 -0700
Subject: [PATCH] Replace many memset calls with struct initialization

This replaces all memset() calls with struct initialization where that
is easily and obviously possible.
---
 contrib/pageinspect/btreefuncs.c |  3 +--
 contrib/pageinspect/ginfuncs.c   | 12 +++-
 contrib/pageinspect/gistfuncs.c  | 12 +++-
 contrib/pageinspect/heapfuncs.c  |  4 +---
 contrib/pageinspect/rawpage.c|  4 +---
 5 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 9375d55e14..a14d83f8cb 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -311,7 +311,7 @@ bt_page_print_tuples(struct user_args *uargs)
boolrightmost = uargs->rightmost;
boolispivottuple;
Datum   values[9];
-   boolnulls[9];
+   boolnulls[9] = {0};
HeapTuple   tuple;
ItemId  id;
IndexTuple  itup;
@@ -331,7 +331,6 @@ bt_page_print_tuples(struct user_args *uargs)
itup = (IndexTuple) PageGetItem(page, id);
 
j = 0;
-   memset(nulls, 0, sizeof(nulls));
values[j++] = DatumGetInt16(offset);
values[j++] = ItemPointerGetDatum(>t_tid);
values[j++] = Int32GetDatum((int) IndexTupleSize(itup));
diff --git a/contrib/pageinspect/ginfuncs.c b/contrib/pageinspect/ginfuncs.c
index 952e9d51a8..6c0183cc63 100644
--- a/contrib/pageinspect/ginfuncs.c
+++ b/contrib/pageinspect/ginfuncs.c
@@ -40,7 +40,7 @@ gin_metapage_info(PG_FUNCTION_ARGS)
GinMetaPageData *metadata;
HeapTuple   resultTuple;
Datum   values[10];
-   boolnulls[10];
+   boolnulls[10] = {0};
 
if (!superuser())
ereport(ERROR,
@@ -75,8 +75,6 @@ gin_metapage_info(PG_FUNCTION_ARGS)
 
metadata = GinPageGetMeta(page);
 
-   memset(nulls, 0, sizeof(nulls));
-
values[0] = Int64GetDatum(metadata->head);
values[1] = Int64GetDatum(metadata->tail);
values[2] = Int32GetDatum(metadata->tailFreeSize);
@@ -107,7 +105,7 @@ gin_page_opaque_info(PG_FUNCTION_ARGS)
GinPageOpaque opaq;
HeapTuple   resultTuple;
Datum   values[3];
-   boolnulls[3];
+   boolnulls[3] = {0};
Datum   flags[16];
int nflags = 0;
uint16  flagbits;
@@ -162,8 +160,6 @@ gin_page_opaque_info(PG_FUNCTION_ARGS)
flags[nflags++] = DirectFunctionCall1(to_hex32, 
Int32GetDatum(flagbits));
}
 
-   memset(nulls, 0, sizeof(nulls));
-
values[0] = Int64GetDatum(opaq->rightlink);
values[1] = Int32GetDatum(opaq->maxoff);
values[2] = PointerGetDatum(construct_array_builtin(flags, nflags, 
TEXTOID));
@@ -255,14 +251,12 @@ gin_leafpage_items(PG_FUNCTION_ARGS)
HeapTuple   resultTuple;
Datum   result;
Datum   values[3];
- 

Re: use ARM intrinsics in pg_lfind32() where available

[Andres Freund]

Hi,

On 2022-08-19 13:08:29 -0700, Nathan Bossart wrote:
> I've tested the patch on a recent macOS (M1 Pro) and Amazon Linux
> (Graviton2), and I've confirmed that the instructions aren't used on a
> Linux/Intel machine.  I did add a new configure check to see if the
> relevant intrinsics are available, but I didn't add a runtime check like
> there is for the CRC instructions since the compilers I used support these
> intrinsics by default.  (I don't think a runtime check would work very well
> with the inline function, anyway.)  AFAICT these intrinsics are pretty
> standard on aarch64, although IIUC the spec indicates that they are
> technically optional.  I suspect that a simple check for "aarch64" would be
> sufficient, but I haven't investigated the level of compiler support yet.

Are you sure there's not an appropriate define for us to use here instead of a
configure test? E.g.

echo|cc -dM -P -E -|grep -iE 'arm|aarch'
...
#define __AARCH64_SIMD__ 1
...
#define __ARM_NEON 1
#define __ARM_NEON_FP 0xE
#define __ARM_NEON__ 1
..

I strikes me as non-scalable to explicitly test all the simd instructions we'd
use.


The story for the CRC checks is different because those instructions often
aren't available with the default compilation flags and aren't guaranteed to
be available at runtime.

Regards,

Andres

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

[Justin Pryzby]

On Wed, Aug 17, 2022 at 09:54:34AM -0500, Justin Pryzby wrote:
> But an unfortunate consequence of not fixing the historic issues is that it
> precludes the possibility that anyone could be expected to notice if they
> introduce more instances of the same problem (as in the first half of these
> patches).  Then the hole which has already been dug becomes deeper, further
> increasing the burden of fixing the historic issues before being able to use
> -Wshadow.
> 
> The first half of the patches fix shadow variables newly-introduced in v15
> (including one of my own patches), the rest are fixing the lowest hanging 
> fruit
> of the "short list" from COPT=-Wshadow=compatible-local
> 
> I can't see that any of these are bugs, but it seems like a good goal to move
> towards allowing use of the -Wshadow* options to help avoid future errors, as
> well as cleanliness and readability (rather than allowing it to get harder to
> use -Wshadow).

+Alvaro

You wrote:

|commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96
|Author: Alvaro Herrera 
|Date:   Fri Mar 23 10:48:22 2018 -0300
|
|Allow FOR EACH ROW triggers on partitioned tables

Which added:

 1  +   partition_recurse = !isInternal && stmt->row &&
 2  +   rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
 3  ...
 4  +   if (partition_recurse)
 5  ...
 6  +   List   *idxs = NIL;
 7  +   List   *childTbls = NIL;
 8  ...
 9  +   if (OidIsValid(indexOid))
10  +   {
11  +   ListCell   *l;
12  +   List   *idxs = NIL;
13  +
14  +   idxs = find_inheritance_children(indexOid, 
ShareRowExclusiveLock);
15  +   foreach(l, idxs)
16  +   childTbls = lappend_oid(childTbls,
17  +   
IndexGetRelation(lfirst_oid(l),
18  +false));
19  +   }
20  ...
21  +   for (i = 0; i < partdesc->nparts; i++)
22  ...
23  +   if (OidIsValid(indexOid))
24  ...
25  +   forboth(l, idxs, l2, childTbls)

The inner idxs is set at line 12, but the outer idxs being looped over at line
25 is still NIL, because the variable is shadowed.

That would be a memory leak or some other bug, except that it also seems to be
dead code ?

https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html#1166

Is it somwhow possible to call CreateTrigger() to create a FOR EACH ROW
trigger, with an index, and not internally ?

The comments say that a user's CREATE TRIGGER will not have a constraint, so
won't have an index.

  * constraintOid is zero when
  * executing a user-entered CREATE TRIGGER command.
  *
+ * indexOid, if nonzero, is the OID of an index associated with the constraint.
+ * We do nothing with this except store it into pg_trigger.tgconstrindid.

See also: <20220817145434.gc26...@telsasoft.com>
Re: shadow variables - pg15 edition

-- 
Justin

Re: Schema variables - new implementation for Postgres 15

[Alvaro Herrera]

> diff --git a/src/backend/parser/parse_relation.c 
> b/src/backend/parser/parse_relation.c
> index f6b740df0a..b3bee39457 100644
> --- a/src/backend/parser/parse_relation.c
> +++ b/src/backend/parser/parse_relation.c
> @@ -3667,8 +3667,8 @@ errorMissingColumn(ParseState *pstate,
>   ereport(ERROR,
>   (errcode(ERRCODE_UNDEFINED_COLUMN),
>relname ?
> -  errmsg("column %s.%s does not exist", relname, 
> colname) :
> -  errmsg("column \"%s\" does not exist", 
> colname),
> +  errmsg("column or variable %s.%s does not 
> exist", relname, colname) :
> +  errmsg("column or variable \"%s\" does not 
> exist", colname),
>state->rfirst ? closestfirst ?
>errhint("Perhaps you meant to reference the 
> column \"%s.%s\".",
>
> state->rfirst->eref->aliasname, closestfirst) :

This is in your patch 12.  I wonder -- if relname is not null, then
surely this is a column and not a variable, right?  So only the second
errmsg() here should be changed, and the first one should remain as in
the original.

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

Re: [PATCH] Optimize json_lex_string by batching character copying

[Nathan Bossart]

On Fri, Aug 19, 2022 at 03:11:36PM +0700, John Naylor wrote:
> This is done. Also:
> - a complete overhaul of the pg_lfind8* tests
> - using a typedef for the vector type
> - some refactoring, name changes and other cleanups (a few of these
> could also be applied to the 32-byte element path, but that is left
> for future work)
> 
> TODO: json-specific tests of the new path

This looks pretty good to me.  Should we rename vector_broadcast() and
vector_has_zero() to indicate that they are working with bytes (e.g.,
vector_broadcast_byte())?  We might be able to use vector_broadcast_int()
in the 32-bit functions, and your other vector functions already have a
_byte suffix.

In general, the approach you've taken seems like a decent readability
improvement.  I'd be happy to try my hand at adjusting the 32-bit path and
adding ARM versions of all this stuff.

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

use ARM intrinsics in pg_lfind32() where available

[Nathan Bossart]

Hi hackers,

This is a follow-up for recent changes that optimized [sub]xip lookups in
XidInMVCCSnapshot() on Intel hardware [0] [1].  I've attached a patch that
uses ARM Advanced SIMD (Neon) intrinsic functions where available to speed
up the search.  The approach is nearly identical to the SSE2 version, and
the usual benchmark [2] shows similar improvements.

  writers  head  simd
  8866   836
  16   849   833
  32   782   822
  64   846   833
  128  805   821
  256  722   739
  512  529   674
  768  374   608
  1024 268   522

I've tested the patch on a recent macOS (M1 Pro) and Amazon Linux
(Graviton2), and I've confirmed that the instructions aren't used on a
Linux/Intel machine.  I did add a new configure check to see if the
relevant intrinsics are available, but I didn't add a runtime check like
there is for the CRC instructions since the compilers I used support these
intrinsics by default.  (I don't think a runtime check would work very well
with the inline function, anyway.)  AFAICT these intrinsics are pretty
standard on aarch64, although IIUC the spec indicates that they are
technically optional.  I suspect that a simple check for "aarch64" would be
sufficient, but I haven't investigated the level of compiler support yet.

Thoughts?

[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b6ef167
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=37a6e5d
[2] https://postgr.es/m/057a9a95-19d2-05f0-17e2-f46ff20e9...@2ndquadrant.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 1295f4d6eedabec1d850893d3bc86180bd33c932 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 19 Aug 2022 10:41:07 -0700
Subject: [PATCH v1 1/1] Use ARM Advanced SIMD intrinsic functions in
 pg_lfind32().

Use ARM Advanced SIMD intrinsic functions to speed up the search,
where available.  Otherwise, use a simple 'for' loop as before.  As
with b6ef167, this speeds up XidInMVCCSnapshot(), but any uses of
pg_lfind32() will also benefit.

Author: Nathan Bossart
---
 config/c-compiler.m4| 25 +++
 configure   | 40 +
 configure.ac|  2 ++
 src/include/pg_config.h.in  |  3 +++
 src/include/port/pg_lfind.h | 35 
 src/include/port/simd.h |  4 
 6 files changed, 109 insertions(+)

diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 69efc5bb10..e8931d7059 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -650,3 +650,28 @@ if test x"$Ac_cachevar" = x"yes"; then
 fi
 undefine([Ac_cachevar])dnl
 ])# PGAC_ARMV8_CRC32C_INTRINSICS
+
+# PGAC_ARM_ADVANCED_SIMD_INTRINSICS
+# -
+# Check if the compiler supports the vdupq_n_u32, vld1q_u32, vceqq_u32,
+# vorrq_u32, and vmaxvq_u32 intrinsic functions.  These instructions were first
+# introduced in ARMv7.
+AC_DEFUN([PGAC_ARM_ADVANCED_SIMD_INTRINSICS],
+[AC_CACHE_CHECK([for vdupq_n_u32, vld1q_u32, vceqq_u32, vorrq_u32, and vmaxvq_u32],
+pgac_cv_arm_advanced_simd_intrinsics,
+[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include ],
+  [unsigned int val[[]] = {1, 2, 3, 4, 5, 6, 7, 8};
+   uint32x4_t keys = vdupq_n_u32(7);
+   uint32x4_t vals1 = vld1q_u32(val);
+   uint32x4_t vals2 = vld1q_u32([[4]]);
+   uint32x4_t tmp1 = vceqq_u32(keys, vals1);
+   uint32x4_t tmp2 = vceqq_u32(keys, vals2);
+   uint32x4_t result = vorrq_u32(tmp1, tmp2);
+   /* return computed value to prevent the above from being optimized away */
+   return vmaxvq_u32(result) != 0;])],
+[pgac_cv_arm_advanced_simd_intrinsics=yes],
+[pgac_cv_arm_advanced_simd_intrinsics=no])])
+if test x"$pgac_cv_arm_advanced_simd_intrinsics" = xyes ; then
+AC_DEFINE(USE_ARM_ADVANCED_SIMD_INTRINSICS, 1,
+  [Define to 1 to use ARM Advanced SIMD (Neon) intrinsics.])
+fi])# PGAC_ARM_ADVANCED_SIMD_INTRINSICS
diff --git a/configure b/configure
index b28fccbc47..0924e5ae8f 100755
--- a/configure
+++ b/configure
@@ -18230,6 +18230,46 @@ $as_echo "slicing-by-8" >&6; }
 fi
 
 
+# Check for ARM Advanced SIMD intrinsics.
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for vdupq_n_u32, vld1q_u32, vceqq_u32, vorrq_u32, and vmaxvq_u32" >&5
+$as_echo_n "checking for vdupq_n_u32, vld1q_u32, vceqq_u32, vorrq_u32, and vmaxvq_u32... " >&6; }
+if ${pgac_cv_arm_advanced_simd_intrinsics+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#include 
+int
+main ()
+{
+unsigned int val[] = {1, 2, 3, 4, 5, 6, 7, 8};
+   uint32x4_t keys = vdupq_n_u32(7);
+   uint32x4_t vals1 = vld1q_u32(val);
+   uint32x4_t vals2 = vld1q_u32([4]);
+   uint32x4_t tmp1 = vceqq_u32(keys, vals1);
+   uint32x4_t tmp2 = vceqq_u32(keys, vals2);
+   uint32x4_t result = vorrq_u32(tmp1, tmp2);
+   /* return computed value to prevent the above from being optimized away */
+   return vmaxvq_u32(result) != 0;
+  ;
+  return 0;
+}
+_ACEOF
+if 

Re: Use array as object (src/fe_utils/parallel_slot.c)

[Ranier Vilela]

Em sex., 19 de ago. de 2022 às 16:22, Justin Pryzby 
escreveu:

> On Fri, Aug 19, 2022 at 03:52:36PM -0300, Ranier Vilela wrote:
> > Em qui., 11 de ago. de 2022 às 09:52, Ranier Vilela 
> escreveu:
> >
> > > Hi,
> > >
> > > One other case suspicious, which I think deserves a conference.
> > > At function wait_on_slots (src/fe_utils/parallel_slot.c)
> > > The variable "slots" are array, but at function call SetCancelConn,
> > > "slots" are used as an object, which at the very least would be
> suspicious.
> >
> > The commit
> >
> https://github.com/postgres/postgres/commit/f71519e545a34ece0a27c8bb1a2b6e197d323163
> > Introduced the affected function.
>
> It's true that the function was added there, but SetCancelConn() was
> called the
> same way before that: SetCancelConn(slots->connection);
>
> If you trace the history back to a17923204, you'll see a comment about the
> "zeroth slot", which makes it clear that the first slot it what's intended.
>
Thank you Justin, for the research.

>
> I agree that it would be clearer if this were written as
> slots[0].connection.
>
But I still think that the new variable introduced,  "cancelconn", became
the real argument.

regards,
Ranier Vilela

Re: Use array as object (src/fe_utils/parallel_slot.c)

[Justin Pryzby]

On Fri, Aug 19, 2022 at 03:52:36PM -0300, Ranier Vilela wrote:
> Em qui., 11 de ago. de 2022 às 09:52, Ranier Vilela  
> escreveu:
> 
> > Hi,
> >
> > One other case suspicious, which I think deserves a conference.
> > At function wait_on_slots (src/fe_utils/parallel_slot.c)
> > The variable "slots" are array, but at function call SetCancelConn,
> > "slots" are used as an object, which at the very least would be suspicious.
>
> The commit
> https://github.com/postgres/postgres/commit/f71519e545a34ece0a27c8bb1a2b6e197d323163
> Introduced the affected function.

It's true that the function was added there, but SetCancelConn() was called the
same way before that: SetCancelConn(slots->connection);

If you trace the history back to a17923204, you'll see a comment about the
"zeroth slot", which makes it clear that the first slot it what's intended.

I agree that it would be clearer if this were written as slots[0].connection.

-- 
Justin

Re: Use array as object (src/fe_utils/parallel_slot.c)

[Ranier Vilela]

Em qui., 11 de ago. de 2022 às 09:52, Ranier Vilela 
escreveu:

> Hi,
>
> One other case suspicious, which I think deserves a conference.
> At function wait_on_slots (src/fe_utils/parallel_slot.c)
> The variable "slots" are array, but at function call SetCancelConn,
> "slots" are used as an object, which at the very least would be suspicious.
>
The commit
https://github.com/postgres/postgres/commit/f71519e545a34ece0a27c8bb1a2b6e197d323163
Introduced the affected function.
I'm not sure you're having problems, but using arrays as single pointer is
not recommended.

regards,
Ranier Vilela

Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory

[mahendrakar s]

Changes look good to me.

Thanks,
Mahendrakar.

On Fri, 19 Aug 2022 at 17:28, Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Fri, Aug 19, 2022 at 1:37 PM mahendrakar s
>  wrote:
> >
> > Hi Bharath,
> > I reviewed your patch. Minor comments.
>
> Thanks.
>
> > 1. Why are we not using durable_unlink instead of unlink to remove the
> partial tmp files?
>
> durable_unlink() issues fsync on the parent directory, if used, those
> fsync() calls will be per partial.tmp file. Moreover, durable_unlink()
> is backend-only, not available for tools i.e. FRONTEND code. If we
> don't durably remove the pratial.tmp file, it will get deleted in the
> next cycle anyways, so no problem there.
>
> > 2. Below could be a simple if(shouldcreatetempfile){} else{} as in error
> case we need to return NULL.
>
> Yeah, that way it is much simpler.
>
> Please review the attached v6 patch.
>
> --
> Bharath Rupireddy
> RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
>

including pid's for `There are XX other sessions using the database`

[Zhihong Yu]

Hi,
Currently errdetail_busy_db() only shows the number of other sessions using
the database but doesn't give any detail about them.
For one of the customers,pg_stat_activity is showing lower number of
connections compared to the number revealed in the error message.

Looking at CountOtherDBBackends(), it seems proc->pid is available
when nbackends is incremented.

I want to poll the community on whether including proc->pid's in the error
message would be useful for troubleshooting.

Thanks

Re: Doc patch

[Bruce Momjian]

On Fri, Oct 15, 2021 at 12:52:48PM -0400, rir wrote:
> 
> In pgsql-docs, this patch has been recommended to you.
> 
> Lacking consensus and so not included is the the deletion of
> comments pointing between the ref/MOVE and FETCH files.  These
> were of the form:
> 
> 

Sorry, I am just looking at this patch.

> diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
> index 4cd4bcba80..1cadec3dc4 100644
> --- a/doc/src/sgml/plpgsql.sgml
> +++ b/doc/src/sgml/plpgsql.sgml
> @@ -3342,7 +3342,7 @@ BEGIN
>   FETCH
>  
>  
> -FETCH  direction { FROM | IN } 
>  cursor INTO 
> target;
> +FETCH  direction   
> FROM | IN  cursor INTO 
> target;
>  

If you look at pl/plpgsql/src/pl_gram.y, you can see in
read_fetch_direction() and complete_direction() that a lot of work is
done to make sure FROM/IN appears (see check_FROM).  Therefore, I think
the above change is incorrect.  Yes, this doesn't match the backend
syntax, probably because of syntax requirements of the PL/pgSQL
language.

>  
> -MOVE  direction { FROM | IN } 
>  cursor;
> +MOVE  direction   
> FROM | IN  cursor;
>  

Same above.

> diff --git a/doc/src/sgml/ref/fetch.sgml b/doc/src/sgml/ref/fetch.sgml
> index ec843f5684..5e19531742 100644
> --- a/doc/src/sgml/ref/fetch.sgml
> +++ b/doc/src/sgml/ref/fetch.sgml
> @@ -27,9 +27,9 @@ PostgreSQL documentation
>   
>  
>  
> -FETCH [ direction [ FROM | IN ] 
> ] cursor_name
> +FETCH [ direction ] [ FROM | IN 
> ] cursor_name
>  
> -where direction can be 
> empty or one of:
> +where direction can one 
> of:
>  
>  NEXT
>  PRIOR
> diff --git a/doc/src/sgml/ref/move.sgml b/doc/src/sgml/ref/move.sgml
> index 4c7d1dca39..c4d859d7b0 100644
> --- a/doc/src/sgml/ref/move.sgml
> +++ b/doc/src/sgml/ref/move.sgml
> @@ -27,9 +27,9 @@ PostgreSQL documentation
>   
>  
>  
> -MOVE [ direction [ FROM | IN ] 
> ] cursor_name
> +MOVE [ direction ] [ FROM | IN 
> ] cursor_name
>  
> -where direction can be 
> empty or one of:
> +where direction can one 
> of:

You are right about the above to changes.  The existing syntax shows
FROM/IN is only possible if a direction is specified, while
src/parser/gram.y says that FROM/IN with no direction is supported.

I plan to apply this second part of the patch soon.

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

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

Re: MultiXact\SLRU buffers configuration

[i . lazarev]

Andrey Borodin wrote 2022-08-18 06:35:


I like the idea of one knob instead of one per each SLRU. Maybe we
even could deduce sane value from NBuffers? That would effectively
lead to 0 knobs :)

Your patch have a prefix "v22-0006", does it mean there are 5 previous
steps of the patchset?

Thank you!


Best regards, Andrey Borodin.


Not sure it's possible to deduce from NBuffers only. 
slru_buffers_scale_shift looks like relief valve for systems with ultra 
scaled NBuffers.


Regarding v22-0006 I just tried to choose index unique for this thread 
so now it's fixed to 0001 indexing.Index: src/include/miscadmin.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
--- a/src/include/miscadmin.h	(revision 020258fbd30d37ddd03d0ec68264d1544f8d2838)
+++ b/src/include/miscadmin.h	(date 1660678108730)
@@ -176,6 +176,7 @@
 extern PGDLLIMPORT int MaxConnections;
 extern PGDLLIMPORT int max_worker_processes;
 extern PGDLLIMPORT int max_parallel_workers;
+extern PGDLLIMPORT int slru_buffers_size_scale;
 
 extern PGDLLIMPORT int MyProcPid;
 extern PGDLLIMPORT pg_time_t MyStartTime;
Index: src/include/access/subtrans.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
diff --git a/src/include/access/subtrans.h b/src/include/access/subtrans.h
--- a/src/include/access/subtrans.h	(revision 020258fbd30d37ddd03d0ec68264d1544f8d2838)
+++ b/src/include/access/subtrans.h	(date 1660678108698)
@@ -12,7 +12,7 @@
 #define SUBTRANS_H
 
 /* Number of SLRU buffers to use for subtrans */
-#define NUM_SUBTRANS_BUFFERS	32
+#define NUM_SUBTRANS_BUFFERS	(32 << slru_buffers_size_scale)
 
 extern void SubTransSetParent(TransactionId xid, TransactionId parent);
 extern TransactionId SubTransGetParent(TransactionId xid);
Index: src/backend/utils/init/globals.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
--- a/src/backend/utils/init/globals.c	(revision 020258fbd30d37ddd03d0ec68264d1544f8d2838)
+++ b/src/backend/utils/init/globals.c	(date 1660678064048)
@@ -150,3 +150,5 @@
 
 int			VacuumCostBalance = 0;	/* working state for vacuum */
 bool		VacuumCostActive = false;
+
+int			slru_buffers_size_scale = 2;	/* power 2 scale for SLRU buffers */
Index: src/backend/access/transam/slru.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
--- a/src/backend/access/transam/slru.c	(revision 020258fbd30d37ddd03d0ec68264d1544f8d2838)
+++ b/src/backend/access/transam/slru.c	(date 1660678063980)
@@ -59,6 +59,7 @@
 #include "pgstat.h"
 #include "storage/fd.h"
 #include "storage/shmem.h"
+#include "port/pg_bitutils.h"
 
 #define SlruFileName(ctl, path, seg) \
 	snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
@@ -71,6 +72,17 @@
  */
 #define MAX_WRITEALL_BUFFERS	16
 
+/*
+ * To avoid overflowing internal arithmetic and the size_t data type, the
+ * number of buffers should not exceed this number.
+ */
+#define SLRU_MAX_ALLOWED_BUFFERS ((1024 * 1024 * 1024) / BLCKSZ)
+
+/*
+ * SLRU bank size for slotno hash banks
+ */
+#define SLRU_BANK_SIZE 8
+
 typedef struct SlruWriteAllData
 {
 	int			num_files;		/* # files actually open */
@@ -134,7 +146,7 @@
 static SlruErrorCause slru_errcause;
 static int	slru_errno;
 
-
+static void SlruAdjustNSlots(int *nslots, int *bankmask);
 static void SimpleLruZeroLSNs(SlruCtl ctl, int slotno);
 static void SimpleLruWaitIO(SlruCtl ctl, int slotno);
 static void SlruInternalWritePage(SlruCtl ctl, int slotno, SlruWriteAll fdata);
@@ -148,6 +160,25 @@
 	  int segpage, void *data);
 static void SlruInternalDeleteSegment(SlruCtl ctl, int segno);
 
+/*
+ * Pick number of slots and bank size optimal for hashed associative SLRU buffers.
+ * We declare SLRU nslots is always power of 2.
+ * We split SLRU to 8-sized hash banks, after some performance benchmarks.
+ * We hash pageno to banks by pageno masked by 3 upper bits.
+ */
+static void
+SlruAdjustNSlots(int *nslots, int *bankmask)
+{
+	Assert(*nslots > 0);
+	Assert(*nslots <= SLRU_MAX_ALLOWED_BUFFERS);
+
+	*nslots = (int) pg_nextpower2_32(Max(SLRU_BANK_SIZE, Min(*nslots, NBuffers / 256)));
+
+	*bankmask = *nslots / SLRU_BANK_SIZE - 1;
+
+	elog(DEBUG5, "nslots %d banksize %d nbanks %d bankmask %x", *nslots, SLRU_BANK_SIZE, *nslots / SLRU_BANK_SIZE, *bankmask);
+}
+
 /*
  * Initialization of shared memory
  */
@@ -156,6 +187,9 @@
 SimpleLruShmemSize(int nslots, int nlsns)
 {
 	Size		sz;
+	int			bankmask_ignore;
+
+	

Re: Issue in postgresql installation - Target version Postgresql 14.

[Bruce Momjian]

First, this is not an appropriate question for hackers.  Second, this is
a question for the package manager where you got the pre-built software.

---

On Fri, Aug 19, 2022 at 03:39:29AM +0530, kavya chandren wrote:
> Dear team,
> 
> We are facing issues during installation of postgresql at our environment.
> 
> 
> This command completed with no errors.
> 
> dnf install -y https://download.postgresql.org/pub/repos/yum/reporpms/
> EL-8-x86_64/pgdg-redhat-repo-latest.noarch.rpm
> 
> Then we ran this command
> 
> dnf install postgresql14-server-14.5-1PGDG.rhel8.x86_64.rpm
> postgresql14-14.5-1PGDG.rhel8.x86_64.rpm
> postgresql14-libs-14.5-1PGDG.rhel8.x86_64.rpm
> 
> and got the following messages
> 
> Updating Subscription Management repositories.
> 
> This system is registered to Red Hat Subscription Management, but is not
> receiving updates. You can use subscription-manager to assign subscriptions.
> 
> Last metadata expiration check: 0:02:07 ago on Thu 18 Aug 2022 03:12:32 PM 
> EDT.
> Error:
>  Problem 1: cannot install the best candidate for the job
>   - nothing provides lz4 needed by postgresql14-14.5-1PGDG.rhel8.x86_64
>  Problem 2: package postgresql14-server-14.5-1PGDG.rhel8.x86_64 requires
> postgresql14(x86-64) = 14.5-1PGDG.rhel8, but none of the providers can be
> installed
>   - cannot install the best candidate for the job
>   - nothing provides lz4 needed by postgresql14-14.5-1PGDG.rhel8.x86_64
> (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to 
> use
> not only best candidate packages)
> 
> Again tried `dnf update` and `dnf install -y postgresql14-server` , but still
> stuck with below error:
> 
> 
> # dnf update
> Updating Subscription Management repositories.
> 
> This system is registered to Red Hat Subscription Management, but is not
> receiving updates. You can useions.
> 
> Last metadata expiration check: 0:42:29 ago on Thu 18 Aug 2022 04:42:25 PM 
> EDT.
> Dependencies resolved.
> ===
> 
>  Package                                   Architecture                Version
> ===
> 
> Upgrading:
>  pg_qualstats_13                           x86_64                    
>  2.0.4-1.rhel8
>  postgresql13                              x86_64                    
>  13.8-1PGDG.rhel8
>  postgresql13-contrib                      x86_64                    
>  13.8-1PGDG.rhel8
>  postgresql13-libs                         x86_64                    
>  13.8-1PGDG.rhel8
>  postgresql13-server                       x86_64                    
>  13.8-1PGDG.rhel8
> 
> Transaction Summary
> ===
> 
> Upgrade  5 Packages
> 
> Total download size: 8.1 M
> Is this ok [y/N]: y
> Downloading Packages:
> (1/5): pg_qualstats_13-2.0.4-1.rhel8.x86_64.rpm
> (2/5): postgresql13-contrib-13.8-1PGDG.rhel8.x86_64.rpm
> (3/5): postgresql13-13.8-1PGDG.rhel8.x86_64.rpm
> (4/5): postgresql13-libs-13.8-1PGDG.rhel8.x86_64.rpm
> (5/5): postgresql13-server-13.8-1PGDG.rhel8.x86_64.rpm
> ---
> Total
> Running transaction check
> Transaction check succeeded.
> Running transaction test
> Transaction test succeeded.
> Running transaction
>   Preparing        :
>   Running scriptlet: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
>   Upgrading        : postgresql13-libs-13.8-1PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
>   Upgrading        : postgresql13-13.8-1PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-13.8-1PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-server-13.8-1PGDG.rhel8.x86_64
>   Upgrading        : postgresql13-server-13.8-1PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-server-13.8-1PGDG.rhel8.x86_64
>   Upgrading        : pg_qualstats_13-2.0.4-1.rhel8.x86_64
>   Running scriptlet: pg_qualstats_13-2.0.4-1.rhel8.x86_64
>   Upgrading        : postgresql13-contrib-13.8-1PGDG.rhel8.x86_64
>   Cleanup          : postgresql13-contrib-13.1-3PGDG.rhel8.x86_64
>   Cleanup          : pg_qualstats_13-2.0.3-1.rhel8.x86_64
>   Running scriptlet: pg_qualstats_13-2.0.3-1.rhel8.x86_64
>   Running scriptlet: postgresql13-server-13.1-3PGDG.rhel8.x86_64
>   Cleanup          : postgresql13-server-13.1-3PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-server-13.1-3PGDG.rhel8.x86_64
>   Cleanup          : postgresql13-13.1-3PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-13.1-3PGDG.rhel8.x86_64
>   Cleanup          : postgresql13-libs-13.1-3PGDG.rhel8.x86_64
>   Running scriptlet: postgresql13-libs-13.1-3PGDG.rhel8.x86_64
>   Verifying        : pg_qualstats_13-2.0.4-1.rhel8.x86_64
>   

Issue in postgresql installation - Target version Postgresql 14.

[kavya chandren]

Dear team,

We are facing issues during installation of postgresql at our environment.

This command completed with no errors.

dnf install -y
https://download.postgresql.org/pub/repos/yum/reporpms/EL-8-x86_64/pgdg-redhat-repo-latest.noarch.rpm

Then we ran this command

dnf install postgresql14-server-14.5-1PGDG.rhel8.x86_64.rpm
postgresql14-14.5-1PGDG.rhel8.x86_64.rpm
postgresql14-libs-14.5-1PGDG.rhel8.x86_64.rpm

and got the following messages

Updating Subscription Management repositories.

This system is registered to Red Hat Subscription Management, but is not
receiving updates. You can use subscription-manager to assign subscriptions.

Last metadata expiration check: 0:02:07 ago on Thu 18 Aug 2022 03:12:32 PM
EDT.
Error:
 Problem 1: cannot install the best candidate for the job
  - nothing provides lz4 needed by postgresql14-14.5-1PGDG.rhel8.x86_64
 Problem 2: package postgresql14-server-14.5-1PGDG.rhel8.x86_64 requires
postgresql14(x86-64) = 14.5-1PGDG.rhel8, but none of the providers can be
installed
  - cannot install the best candidate for the job
  - nothing provides lz4 needed by postgresql14-14.5-1PGDG.rhel8.x86_64
(try to add '--skip-broken' to skip uninstallable packages or '--nobest' to
use not only best candidate packages)
*Again tried `dnf update` and `dnf install -y postgresql14-server` , but
still stuck with below error:*

# dnf update
Updating Subscription Management repositories.

This system is registered to Red Hat Subscription Management, but is not
receiving updates. You can useions.

Last metadata expiration check: 0:42:29 ago on Thu 18 Aug 2022 04:42:25 PM
EDT.
Dependencies resolved.
===
 Package   Architecture
 Version
===
Upgrading:
 pg_qualstats_13   x86_64
 2.0.4-1.rhel8
 postgresql13  x86_64
 13.8-1PGDG.rhel8
 postgresql13-contrib  x86_64
 13.8-1PGDG.rhel8
 postgresql13-libs x86_64
 13.8-1PGDG.rhel8
 postgresql13-server   x86_64
 13.8-1PGDG.rhel8

Transaction Summary
===
Upgrade  5 Packages

Total download size: 8.1 M
Is this ok [y/N]: y
Downloading Packages:
(1/5): pg_qualstats_13-2.0.4-1.rhel8.x86_64.rpm
(2/5): postgresql13-contrib-13.8-1PGDG.rhel8.x86_64.rpm
(3/5): postgresql13-13.8-1PGDG.rhel8.x86_64.rpm
(4/5): postgresql13-libs-13.8-1PGDG.rhel8.x86_64.rpm
(5/5): postgresql13-server-13.8-1PGDG.rhel8.x86_64.rpm
---
Total
Running transaction check
Transaction check succeeded.
Running transaction test
Transaction test succeeded.
Running transaction
  Preparing:
  Running scriptlet: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
  Upgrading: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
  Upgrading: postgresql13-13.8-1PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-13.8-1PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-server-13.8-1PGDG.rhel8.x86_64
  Upgrading: postgresql13-server-13.8-1PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-server-13.8-1PGDG.rhel8.x86_64
  Upgrading: pg_qualstats_13-2.0.4-1.rhel8.x86_64
  Running scriptlet: pg_qualstats_13-2.0.4-1.rhel8.x86_64
  Upgrading: postgresql13-contrib-13.8-1PGDG.rhel8.x86_64
  Cleanup  : postgresql13-contrib-13.1-3PGDG.rhel8.x86_64
  Cleanup  : pg_qualstats_13-2.0.3-1.rhel8.x86_64
  Running scriptlet: pg_qualstats_13-2.0.3-1.rhel8.x86_64
  Running scriptlet: postgresql13-server-13.1-3PGDG.rhel8.x86_64
  Cleanup  : postgresql13-server-13.1-3PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-server-13.1-3PGDG.rhel8.x86_64
  Cleanup  : postgresql13-13.1-3PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-13.1-3PGDG.rhel8.x86_64
  Cleanup  : postgresql13-libs-13.1-3PGDG.rhel8.x86_64
  Running scriptlet: postgresql13-libs-13.1-3PGDG.rhel8.x86_64
  Verifying: pg_qualstats_13-2.0.4-1.rhel8.x86_64
  Verifying: pg_qualstats_13-2.0.3-1.rhel8.x86_64
  Verifying: postgresql13-13.8-1PGDG.rhel8.x86_64
  Verifying: postgresql13-13.1-3PGDG.rhel8.x86_64
  Verifying: postgresql13-contrib-13.8-1PGDG.rhel8.x86_64
  Verifying: postgresql13-contrib-13.1-3PGDG.rhel8.x86_64
  Verifying: postgresql13-libs-13.8-1PGDG.rhel8.x86_64
  Verifying: postgresql13-libs-13.1-3PGDG.rhel8.x86_64
  Verifying: postgresql13-server-13.8-1PGDG.rhel8.x86_64
  Verifying: postgresql13-server-13.1-3PGDG.rhel8.x86_64
Installed products updated.

Upgraded:
  

Re: Trivial doc patch

[Bruce Momjian]

On Sat, Oct 16, 2021 at 01:11:49PM -0400, rir wrote:
> On Sat, Oct 16, 2021 at 11:14:46AM +0900, Michael Paquier wrote:
> > On Fri, Oct 15, 2021 at 01:13:14PM -0400, rir wrote:
> > > This removes the outer square brackets in the create_database.sgml
> > > file's synopsis.  In the command sysopses, this is the only case
> > > where an optional group contains only optional groups.
> > >
> > >  CREATE DATABASE name
> > > -[ [ WITH ] [ OWNER [=]  > > class="parameter">user_name ]
> > > +[ WITH ] [ OWNER [=]  > > class="parameter">user_name ]
> > > [...]
> > > -   [ IS_TEMPLATE [=]  > > class="parameter">istemplate ] ]
> > > +   [ IS_TEMPLATE [=]  > > class="parameter">istemplate ]
> > >  
> > >   
> > 
> > You are not wrong, and the existing doc is not wrong either.  I tend
> > to prefer the existing style, though, as it insists on the options
> > as being a single group, with or without the keyword WITH.
> 
> Michael, perhaps I mistake you; it seems you would like it better with
> the extra '[' before OWNER.  That would more accurately point up
> 
> CREATE DATABASE name WITH;
> 
> Either way, my argument would have the basis.
> 
> In what sense are the options a single group?  That they all might
> follow the 'WITH' is expressed without the duplicated brackets.
> That the extra braces promote readability relies on an assumption or
> knowledge of the command.
> 
> Given that 'optional, optional' has no independent meaning from
> 'optional';  it requires one to scan the entire set looking for
> the non-optional embedded in the option.  So no gain.

I originally had the same reaction Michael Paquier did, that having one
big optional block is nice, but seeing that 'CREATE DATABASE name WITH'
actually works, I can see the point in having our syntax be accurate,
and removing the outer optional brackets now does seem like an
improvement to me.

Attached is the proposed change.

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

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

diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index 0b32e7ecf9..0ce0bd8a1a 100644
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  
 
 CREATE DATABASE name
-[ [ WITH ] [ OWNER [=] user_name ]
+[ WITH ] [ OWNER [=] user_name ]
[ TEMPLATE [=] template ]
[ ENCODING [=] encoding ]
[ STRATEGY [=] strategy ] ]
@@ -36,7 +36,7 @@ CREATE DATABASE name
[ ALLOW_CONNECTIONS [=] allowconn ]
[ CONNECTION LIMIT [=] connlimit ]
[ IS_TEMPLATE [=] istemplate ]
-   [ OID [=] oid ] ]
+   [ OID [=] oid ]
 
  
 

Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Ranier Vilela]

Em sex., 19 de ago. de 2022 às 10:28, Tom Lane  escreveu:

> Ranier Vilela  writes:
> > At function parallel_vacuum_process_all_indexes there is
> > a typo with a logical connector.
> > I think that correct is &&, because both of the operators are
> > bool types [1].
> > As a result, parallel vacuum workers can be incorrectly enabled.
>
> Since they're bools, the C spec requires them to promote to integer
> 0 or 1, therefore the & operator will yield the desired result.
> So there's not going to be any incorrect behavior.

It seems that you are right.

#include 

#ifdef __cplusplus
extern "C" {
#endif

int main()
{
bool op1 = false;
bool op2 = true;
bool band;
bool cand;

band = op1 & op2;
printf("res=%d\n", band);

cand = op1 && op2;
printf("res=%d\n", cand);
}

#ifdef __cplusplus
}
#endif

results:
res=0
res=0

So, my assumption is incorrect.

regards,
Ranier Vilela

Re: SQL/JSON features for v15

[Jonathan S. Katz]

Hi,

On 8/17/22 11:45 PM, Nikita Glukhov wrote:

Hi,

On 17.08.2022 04:45, Jonathan S. Katz wrote:


On 8/15/22 10:14 PM, Andres Freund wrote:

I pushed a few cleanups to 
https://github.com/anarazel/postgres/commits/json
while I was hacking on this (ignore that it's based on the meson 
tree, that's
just faster for me). Some of them might not be applicable anymore, 
but it

might still make sense for you to look at.


With RMT hat on, this appears to be making progress. A few questions / 
comments for the group:


1. Nikita: Did you have a chance to review Andres's changes as well?


Yes, I have reviewed Andres's changes, they all are ok.


Thank you!


Then I started to do on the top of it other fixes that help to avoid
subtransactions when they are not needed. And it ended in the new
refactoring of coercion code.  Also I moved here from v6-0003 fix of
ExecEvalJsonNeedSubtransaction() which considers more cases.


Great.

Andres, Robert: Do these changes address your concerns about the use of 
substransactions and reduce the risk of xid wraparound?



On 16.08.2022 05:14, Andres Freund wrote:

But for JIT I still had to construct additional ExprState with a
function compiled from subexpression steps.



Why did you have to do this?


I simply did not dare to implement compilation of recursively-callable
function with additional parameter stepno.  In the v8 patch I did it
by adding a switch with all possible jump addresses of EEOP_SUBTRANS
steps in the beginning of the function.  And it really seems to work
faster, but needs more exploration.  See patch 0003, where both
variants preserved using #ifdef.


The desciprion of the v7 patches:

0001 Simplify JsonExpr execution
  Andres's changes + mine:
   - Added JsonCoercionType enum, fields like via_io replaced with it
   - Emit only context item steps in JSON_TABLE_OP case
   - Skip coercion of NULLs to non-domain types (is it correct?)

0002 Fix returning of json[b] domains in JSON_VALUE:
   simply rebase of v6 onto 0001

0003 Add EEOP_SUBTRANS executor step
   v6 + new recursive JIT

0004 Split JsonExpr execution into steps
   simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR


What do folks think of these patches?

Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: Strip -mmacosx-version-min options from plperl build

[Tom Lane]

Peter Eisentraut  writes:
> After analyzing the source code of ExtUtils::Embed's ldopts, I think we 
> can also do this by subtracting $Config{ldflags}, since
> my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";
> and we really just want the $ld_or_bs part. (@archives should be empty 
> for our uses.)

+1, this looks like a nice clean solution.  I see that it gets rid
of stuff we don't really want on RHEL8 as well as various generations
of macOS.

> This would get rid of -mmacosx-version-min and -arch and all the things 
> you showed, including -L/opt/local/lib, which is probably there so that 
> the build of Perl itself could look there for things, but we don't need it.

It is a little weird that they are inserting -L/opt/local/lib or
-L/usr/local/lib on so many different platforms.  But I concur
that if we need that, we likely should be inserting it ourselves
rather than absorbing it from their $ldflags.

BTW, I think the -arch business is dead code anyway now that we
desupported PPC-era macOS; I do not see any such switches from
modern macOS' perl.  So not having a special case for that is an
additional win.

Patch LGTM; I noted only a trivial typo in the commit message:

-like we already do with $Config{ccdlflags}.  Those flags the choices
+like we already do with $Config{ccdlflags}.  Those flags are the choices

regards, tom lane

Re: MERGE and parsing with prepared statements

[Justin Pryzby]

On Fri, Aug 12, 2022 at 01:53:25PM +0200, Alvaro Herrera wrote:
> On 2022-Aug-12, Simon Riggs wrote:
> 
> > Sorry, but I disagree with this chunk in the latest commit,
> > specifically, changing the MATCHED from after to before the NOT
> > MATCHED clause.

3d895bc84 also moved a semicolon into the middle of the sql statement.

> > The whole point of the second example was to demonstrate that the
> > order of the MATCHED/NOT MATCHED clauses made no difference.
> > 
> > By changing the examples so they are the same, the sentence at line
> > 573 now makes no sense.
> 
> Hmm, I thought the point of the example was to show that you can replace
> the table in the USING clause with a query that retrieves the column;
> but you're right, we lost the thing there.  Maybe it was too subtle to
> the point that I failed to understand it.  Perhaps we can put it back
> the way it was and explain these two differences (change of data source
> *and* clause ordering) more explicitly.

Evidently I misunderstood it, too.

-- 
Justin

Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Tom Lane]

Ranier Vilela  writes:
> At function parallel_vacuum_process_all_indexes there is
> a typo with a logical connector.
> I think that correct is &&, because both of the operators are
> bool types [1].
> As a result, parallel vacuum workers can be incorrectly enabled.

Since they're bools, the C spec requires them to promote to integer
0 or 1, therefore the & operator will yield the desired result.
So there's not going to be any incorrect behavior.  Nonetheless,
I agree that && would be better, because it would short-circuit
the evaluation of parallel_vacuum_index_is_parallel_safe() when
there's no need.

regards, tom lane

Re: Reducing planning time on tables with many indexes

[David Geier]

On 8/1/22 15:33, David Geier wrote:

Hi Tom,

On Wed, Jul 27, 2022 at 7:15 PM Tom Lane  wrote:

I wrote:
> Unfortunately, as things stand today, the planner needs more
than the
> right to look at the indexes' schemas, because it makes physical
accesses
> to btree indexes to find out their tree height (and I think
there are some
> comparable behaviors in other AMs).  I've never particularly
cared for
> that implementation, and would be glad to rip out that behavior
if we can
> find another way.  Maybe we could persuade VACUUM or ANALYZE to
store that
> info in the index's pg_index row, or some such, and then the planner
> could use it with no lock?

It seems like _bt_getrootheight() first checks if the height is cached 
and only if it isn't it accesses index meta pages.
If the index locks are only taken for the sake of _bt_getrootheight() 
accessing index meta pages in case they are not cached, maybe the 
index locks could be taken conditionally.
However, postponing the call to where it is really needed sounds even 
better.



A first step here could just be to postpone fetching
_bt_getrootheight()
until we actually need it during cost estimation.  That would
avoid the
need to do it at all for indexes that indxpath.c discards as
irrelevant,
which is a decision made on considerably more information than the
proposed patch uses.


Hi Tom,

I gave the idea of moving _bt_getrootheight() into costsize.c and 
filling IndexOptInfo in get_relation_info() via syscache instead of 
relcache a try, but didn't get very far.
Moving out _bt_getrootheight() was straightforward, and we should do 
nevertheless. However, it seems like get_relation_info() strongly 
depends on the index's Relation for quite some stuff. A fair amount of 
fields I could actually fill from syscache, but there are some that 
either need data not stored in syscache (e.g. estimate_rel_size(), 
Relation::rd_smgr needed by RelationGetNumberOfBlocksInFork()) or need 
fields that are cached in the index's Relation and would have to be 
recomputed otherwise (e.g. Relation::rd_indexprs filled by 
RelationGetIndexExpressions(), Relation::rd_indpred filled by 
RelationGetIndexPredicate()). Even if we could somehow obtain the 
missing info from somewhere, recomputing the otherwise cached fields 
from Relation would likely cause a significant slowdown in the serial case.


Beyond that I did some off-CPU profiling to precisely track down which 
lock serializes execution. It turned out to be the MyProc::fpInfoLock 
lightweight lock. This lock is used in the fast path of the heavyweight 
lock. In the contenting case, fpInfoLock is acquired in LW_EXCLUSIVE 
mode to (1) check if there is no other process holding a stronger lock, 
and if not, to reserve a process local fast path lock slot and (2) to 
return the fast path lock slots all in one go. To do so, the current 
implementation always linearly iterates over all lock slots. The 
corresponding call stacks are:


get_relation_info()   CommitTransaction()
  index_open()  ResourceOwnerRelease()
    relation_open() ResourceOwnerReleaseInternal()
  LockRelationOid() ProcReleaseLocks()
    LockAcquireExtended() LockReleaseAll() <-- called 
twice from ProcReleaseLocks()

  LWLockAcquire()

On top of that there are only 16 fast path lock slots. One slot is 
always taken up by the parent relation, leaving only 15 slots for the 
indexes. As soon as a session process runs out of slots, it falls back 
to the normal lock path which has to mess around with the lock table. To 
do so it also acquires a lightweight lock in LW_EXCLUSIVE mode. This 
lightweight lock however is partitioned and therefore does not content. 
Hence, normal lock acquisition is slower but contents less.


To prove above findings I increased the number of fast path lock slots 
per connection and optimized FastPathGrantRelationLock() and 
FastPathUnGrantRelationLock(). With these changes the lock contention 
disappeared and the workload scales linearly (the code I tested with 
also included moving out _bt_getrootheight()):


| Parallel streams | TPS  | TPS / stream  |
|--|--|---|
| 1    |   5,253  | 5,253 |
| 10   |  51,406  | 5,140 |
| 20   | 101,401  | 5,070 |
| 30   | 152,023  | 5,067 |
| 40   | 200,607  | 5,015 |
| 50   | 245,359  | 4,907 |
| 60   | 302,994  | 5,049 |

However, with the very same setup, the index filtering approach yields 
486k TPS with 60 streams and 9,827 TPS with a single stream. The single 
stream number shows that this is not because it scales even better, but 
just because less work is spent during planning. A quick perf session 
showed that a fair amount of time is spent to get the 

Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Amit Kapila]

On Fri, Aug 19, 2022 at 5:40 PM Bharath Rupireddy
 wrote:
>
> On Fri, Aug 19, 2022 at 5:35 PM Ranier Vilela  wrote:
> >
> > Hi,
> >
> > At function parallel_vacuum_process_all_indexes there is
> > a typo with a logical connector.
> >
> > I think that correct is &&, because both of the operators are
> > bool types [1].
> >
> > As a result, parallel vacuum workers can be incorrectly enabled.
> >
> > Attached a trivial fix.
> >
> > [1] 
> > https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand
>
> Good catch! Patch LGTM.
>

+1. This looks fine to me as well. I'll take care of this early next
week unless someone thinks otherwise.

-- 
With Regards,
Amit Kapila.

Fix a comment in WalSnd structure

[Bharath Rupireddy]

Hi,

WalSnd structure mutex is being used to protect all the variables of
that structure, not just 'variables shown above' [1]. A tiny patch
attached to fix the comment.

Thoughts?

[1]
diff --git a/src/include/replication/walsender_private.h
b/src/include/replication/walsender_private.h
index c14888e493..9c61f92c44 100644
--- a/src/include/replication/walsender_private.h
+++ b/src/include/replication/walsender_private.h
@@ -65,7 +65,7 @@ typedef struct WalSnd
 */
int sync_standby_priority;

-   /* Protects shared variables shown above. */
+   /* Protects shared variables in this structure. */
slock_t mutex;

--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
From 58ef0c3165a9a12718af6173d42479c6e8a756de Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Wed, 17 Aug 2022 11:42:51 +
Subject: [PATCH v1] Fix a comment in WalSnd structure

WalSnd structure mutex is being used to protect all the variables
of that structure, not just 'variables shown above'. Correct the
comment.
---
 src/include/replication/walsender_private.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/replication/walsender_private.h b/src/include/replication/walsender_private.h
index c14888e493..9c61f92c44 100644
--- a/src/include/replication/walsender_private.h
+++ b/src/include/replication/walsender_private.h
@@ -65,7 +65,7 @@ typedef struct WalSnd
 	 */
 	int			sync_standby_priority;
 
-	/* Protects shared variables shown above. */
+	/* Protects shared variables in this structure. */
 	slock_t		mutex;
 
 	/*
-- 
2.34.1

Re: Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Bharath Rupireddy]

On Fri, Aug 19, 2022 at 5:35 PM Ranier Vilela  wrote:
>
> Hi,
>
> At function parallel_vacuum_process_all_indexes there is
> a typo with a logical connector.
>
> I think that correct is &&, because both of the operators are
> bool types [1].
>
> As a result, parallel vacuum workers can be incorrectly enabled.
>
> Attached a trivial fix.
>
> [1] 
> https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand

Good catch! Patch LGTM.

-- 
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/

Re: pg_receivewal and SIGTERM

[Bharath Rupireddy]

On Fri, Aug 19, 2022 at 4:24 PM Christoph Berg  wrote:
>
> Re: Bharath Rupireddy
> > pg_receivewal will exit with status 0 when
> > -   terminated by the SIGINT signal.  (That is the
> > +   terminated by the SIGINT or
> > +   SIGTERM signal.  (That is the
> > normal way to end it.  Hence it is not an error.)  For fatal errors or
> > other signals, the exit status will be nonzero.
> >
> > Can we specify the reason in the docs why a SIGTERM causes (which
> > typically would cause a program to end with non-zero exit code)
> > pg_receivewal and pg_recvlogical exit with zero exit code? Having this
> > in the commit message would help developers but the documentation will
> > help users out there.
>
> We could add "because you want that if it's running as a daemon", but

+1 to add "some" info in the docs (I'm not sure about the better
wording though), we can try to be more specific of the use case if
required.

-- 
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/

Fix typo with logical connector (src/backend/commands/vacuumparallel.c)

[Ranier Vilela]

Hi,

At function parallel_vacuum_process_all_indexes there is
a typo with a logical connector.

I think that correct is &&, because both of the operators are
bool types [1].

As a result, parallel vacuum workers can be incorrectly enabled.

Attached a trivial fix.

regards,
Ranier Vilela

[1]
https://wiki.sei.cmu.edu/confluence/display/c/EXP46-C.+Do+not+use+a+bitwise+operator+with+a+Boolean-like+operand


001-avoid-call-function-typo-shortcut.patch
Description: Binary data

Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory

[Bharath Rupireddy]

On Fri, Aug 19, 2022 at 1:37 PM mahendrakar s
 wrote:
>
> Hi Bharath,
> I reviewed your patch. Minor comments.

Thanks.

> 1. Why are we not using durable_unlink instead of unlink to remove the 
> partial tmp files?

durable_unlink() issues fsync on the parent directory, if used, those
fsync() calls will be per partial.tmp file. Moreover, durable_unlink()
is backend-only, not available for tools i.e. FRONTEND code. If we
don't durably remove the pratial.tmp file, it will get deleted in the
next cycle anyways, so no problem there.

> 2. Below could be a simple if(shouldcreatetempfile){} else{} as in error case 
> we need to return NULL.

Yeah, that way it is much simpler.

Please review the attached v6 patch.

--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/


v6-0001-Make-WAL-file-initialization-by-pg_receivewal-ato.patch
Description: Binary data

Re: logical decoding and replication of sequences, take 2

[Tomas Vondra]

I've been thinking about the two optimizations mentioned at the end a
bit more, so let me share my thoughts before I forget that:

On 8/18/22 23:10, Tomas Vondra wrote:
>
> ...
>
> And maybe we could then use the LSN to read the increment from the WAL
> during decoding, instead of having to read it and WAL-log it during
> commit. Essentially, we'd run a local XLogReader. Of course, we'd have
> to be careful about checkpoints, not sure what to do about that.
> 

I think logging just the LSN is workable.

I was worried about dealing with checkpoints, because imagine you do
nextval() on sequence that was last WAL-logged a couple checkpoints
back. Then you wouldn't be able to read the LSN (when decoding), because
the WAL might have been recycled. But that can't happen, because we
always force WAL-logging the first time nextval() is called after a
checkpoint. So we know the LSN is guaranteed to be available.

Of course, this would not reduce the amount of WAL messages, because
we'd still log all sequences touched by the transaction. We wouldn't
need to read the state from disk, though, and we could ignore "old"
stuff in decoding (with LSN lower than the last LSN we decoded).

For frequently used sequences that seems like a win.


> Another idea that just occurred to me is that if we end up having to
> read the sequence state during commit, maybe we could at least optimize
> it somehow. For example we might track LSN of the last logged state for
> each sequence (in shared memory or something), and the other sessions
> could just skip the WAL-log if their "local" LSN is <= than this LSN.
> 

Tracking the last LSN for each sequence (in a SLRU or something) should
work too, I guess. In principle this just moves the skipping of "old"
increments from decoding to writing, so that we don't even have to write
those into WAL.

We don't even need persistence, nor to keep all the records, I think. If
you don't find a record for a given sequence, assume it wasn't logged
yet and just log it. Of course, it requires a bit of shared memory for
each sequence, say ~32B. Not sure about the overhead, but I'd bet if you
have many (~thousands) frequently used sequences, there'll be a lot of
other overhead making this irrelevant.

Of course, if we're doing the skipping when writing the WAL, maybe we
should just read the sequence state - we'd do the I/O, but only in
fraction of the transactions, and we wouldn't need to read old WAL in
logical decoding.


regards

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

Re: pg_receivewal and SIGTERM

[Christoph Berg]

Re: Bharath Rupireddy
> pg_receivewal will exit with status 0 when
> -   terminated by the SIGINT signal.  (That is the
> +   terminated by the SIGINT or
> +   SIGTERM signal.  (That is the
> normal way to end it.  Hence it is not an error.)  For fatal errors or
> other signals, the exit status will be nonzero.
> 
> Can we specify the reason in the docs why a SIGTERM causes (which
> typically would cause a program to end with non-zero exit code)
> pg_receivewal and pg_recvlogical exit with zero exit code? Having this
> in the commit message would help developers but the documentation will
> help users out there.

We could add "because you want that if it's running as a daemon", but
TBH, I'd rather remove the parentheses part. It sounds too much like
"it works that way because that way is the sane way".

Christoph

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

[Amit Kapila]

On Fri, Aug 19, 2022 at 3:05 PM Peter Smith  wrote:
>
> On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila  wrote:
> >
> > On Fri, Aug 19, 2022 at 2:36 PM Peter Smith  wrote:
> > >
> > > Here are my review comments for the v23-0005 patch:
> > >
> > > ==
> > >
> > > Commit Message says:
> > > main_worker_pid is Process ID of the main apply worker, if this process 
> > > is a
> > > apply background worker. NULL if this process is a main apply worker or a
> > > synchronization worker.
> > > The new column can make it easier to distinguish main apply worker and 
> > > apply
> > > background worker.
> > >
> > > --
> > >
> > > Having a column called ‘main_worker_pid’ which is defined to be NULL
> > > if the process *is* the main apply worker does not make any sense to
> > > me.
> > >
> >
> > I haven't read this part of a patch but it seems to me we have
> > something similar for parallel query workers. Refer 'leader_pid'
> > column in pg_stat_activity.
> >
>
> IIUC (from the patch 0005 commit message) the intention is to be able
> to easily distinguish the worker types.
>

I think it is only to distinguish between leader apply worker and
background apply workers. The tablesync worker can be distinguished
based on relid field.

-- 
With Regards,
Amit Kapila.

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

[Peter Smith]

On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila  wrote:
>
> On Fri, Aug 19, 2022 at 2:36 PM Peter Smith  wrote:
> >
> > Here are my review comments for the v23-0005 patch:
> >
> > ==
> >
> > Commit Message says:
> > main_worker_pid is Process ID of the main apply worker, if this process is a
> > apply background worker. NULL if this process is a main apply worker or a
> > synchronization worker.
> > The new column can make it easier to distinguish main apply worker and apply
> > background worker.
> >
> > --
> >
> > Having a column called ‘main_worker_pid’ which is defined to be NULL
> > if the process *is* the main apply worker does not make any sense to
> > me.
> >
>
> I haven't read this part of a patch but it seems to me we have
> something similar for parallel query workers. Refer 'leader_pid'
> column in pg_stat_activity.
>

IIUC (from the patch 0005 commit message) the intention is to be able
to easily distinguish the worker types.

I thought using a leader PID (by whatever name) seemed a poor way to
achieve that in this case because the PID is either NULL or not NULL,
but there are 3 kinds of subscription workers, not 2.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

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

[Amit Kapila]

On Fri, Aug 19, 2022 at 2:36 PM Peter Smith  wrote:
>
> Here are my review comments for the v23-0005 patch:
>
> ==
>
> Commit Message says:
> main_worker_pid is Process ID of the main apply worker, if this process is a
> apply background worker. NULL if this process is a main apply worker or a
> synchronization worker.
> The new column can make it easier to distinguish main apply worker and apply
> background worker.
>
> --
>
> Having a column called ‘main_worker_pid’ which is defined to be NULL
> if the process *is* the main apply worker does not make any sense to
> me.
>

I haven't read this part of a patch but it seems to me we have
something similar for parallel query workers. Refer 'leader_pid'
column in pg_stat_activity.

-- 
With Regards,
Amit Kapila.

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

[Peter Smith]

Here are my review comments for the v23-0005 patch:

==

Commit Message says:
main_worker_pid is Process ID of the main apply worker, if this process is a
apply background worker. NULL if this process is a main apply worker or a
synchronization worker.
The new column can make it easier to distinguish main apply worker and apply
background worker.

--

Having a column called ‘main_worker_pid’ which is defined to be NULL
if the process *is* the main apply worker does not make any sense to
me.

IMO it feels hacky trying to squeeze meaning out of the
'main_worker_pid' member of the LogicalRepWorker like this.

If the intention really is to make it easier to distinguish the
different kinds of subscription workers then surely there are much
better ways to achieve that. For example, why not introduce a new
'type' enum member of the LogicalRepWorker (e.g.
WORKER_TYPE_TABLESYNC='t', WORKER_TYPE_APPLY='a',
WORKER_TYPE_PARALLEL_APPLY='p'), then use some char column to expose
it? As a bonus, I think the other code (i.e.patch 0001) will also be
improved if a 'type' member is added.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

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

[Amit Kapila]

On Thu, Aug 18, 2022 at 5:14 PM Amit Kapila  wrote:
>
> On Wed, Aug 17, 2022 at 11:58 AM wangw.f...@fujitsu.com
>  wrote:
> >
> > Attach the new patches.
> >
>
> Few comments on v23-0001
> ===
>

Some more comments on v23-0001

1.
static bool
 handle_streamed_transaction(LogicalRepMsgType action, StringInfo s)
{
...
- /* not in streaming mode */
- if (!in_streamed_transaction)
+ /* Not in streaming mode and not in apply background worker. */
+ if (!(in_streamed_transaction || am_apply_bgworker()))
  return false;

This check appears a bit strange because ideally in bgworker
in_streamed_transaction should be false. I think we should set
in_streamed_transaction to true in apply_handle_stream_start() only
when we are going to write to file. Is there a reason for not doing
the same?

2.
+ {
+ /* This is the main apply worker. */
+ ApplyBgworkerInfo *wstate = apply_bgworker_find(xid);
+
+ /*
+ * Check if we are processing this transaction using an apply
+ * background worker and if so, send the changes to that worker.
+ */
+ if (wstate)
+ {
+ /* Send STREAM ABORT message to the apply background worker. */
+ apply_bgworker_send_data(wstate, s->len, s->data);

Why at some places the patch needs to separately fetch
ApplyBgworkerInfo whereas at other places it directly uses
stream_apply_worker to pass the data to bgworker.

3. Why apply_handle_stream_abort() or apply_handle_stream_prepare()
doesn't use apply_bgworker_active() to identify whether it needs to
send the information to bgworker?

4. In apply_handle_stream_prepare(), apply_handle_stream_abort(), and
some other similar functions, the patch handles three cases (a) apply
background worker, (b) sending data to bgworker, (c) handling for
streamed transaction in apply worker. I think the code will look
better if you move the respective code for all three cases into
separate functions. Surely, if the code to deal with each of the cases
is less then we don't need to move it to a separate function.

5.
@@ -1088,24 +1177,78 @@ apply_handle_stream_prepare(StringInfo s)
{
...
+ in_remote_transaction = false;
+
+ /* Unlink the files with serialized changes and subxact info. */
+ stream_cleanup_files(MyLogicalRepWorker->subid, prepare_data.xid);
+ }
+ }

  in_remote_transaction = false;
...

We don't need to in_remote_transaction to false in multiple places.

6.
@@ -1177,36 +1311,93 @@ apply_handle_stream_start(StringInfo s)
{
...
...
+ if (am_apply_bgworker())
  {
- MemoryContext oldctx;
-
- oldctx = MemoryContextSwitchTo(ApplyContext);
+ /*
+ * Make sure the handle apply_dispatch methods are aware we're in a
+ * remote transaction.
+ */
+ in_remote_transaction = true;

- MyLogicalRepWorker->stream_fileset = palloc(sizeof(FileSet));
- FileSetInit(MyLogicalRepWorker->stream_fileset);
+ /* Begin the transaction. */
+ AcceptInvalidationMessages();
+ maybe_reread_subscription();

- MemoryContextSwitchTo(oldctx);
+ StartTransactionCommand();
+ BeginTransactionBlock();
+ CommitTransactionCommand();
  }
...

Why do we need to start a transaction here? Why can't it be done via
begin_replication_step() during the first operation apply? Is it
because we may need to define a save point in bgworker and we don't
that information beforehand? If so, then also, can't it be handled by
begin_replication_step() either by explicitly passing the information
or checking it there and then starting a transaction block? In any
case, please add a few comments to explain why this separate handling
is required for bgworker?

7. When we are already setting bgworker status as APPLY_BGWORKER_BUSY
in apply_bgworker_setup_dsm() then why do we need to set it again in
apply_bgworker_start()?

8. It is not clear to me how APPLY_BGWORKER_EXIT status is used. Is it
required for the cases where bgworker exists due to some error and
then apply worker uses it to detect that and exits? How other
bgworkers would notice this, is it done via
apply_bgworker_check_status()?

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Optimize json_lex_string by batching character copying

[John Naylor]

On Tue, Aug 16, 2022 at 4:23 AM Nathan Bossart  wrote:
>
> On Mon, Aug 15, 2022 at 08:33:21PM +0700, John Naylor wrote:
> > +#ifdef USE_SSE2
> > + chunk = _mm_loadu_si128((const __m128i *) [i]);
> > +#else
> > + memcpy(, [i], sizeof(chunk));
> > +#endif   /* USE_SSE2 */
>
> Perhaps there should be a macro or inline function for loading a vector so
> that these USE_SSE2 checks can be abstracted away, too.

This is done. Also:
- a complete overhaul of the pg_lfind8* tests
- using a typedef for the vector type
- some refactoring, name changes and other cleanups (a few of these
could also be applied to the 32-byte element path, but that is left
for future work)

TODO: json-specific tests of the new path

--
John Naylor
EDB: http://www.enterprisedb.com
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index fefd1d24d9..efcaedd682 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -19,6 +19,7 @@
 
 #include "common/jsonapi.h"
 #include "mb/pg_wchar.h"
+#include "port/pg_lfind.h"
 
 #ifndef FRONTEND
 #include "miscadmin.h"
@@ -844,7 +845,7 @@ json_lex_string(JsonLexContext *lex)
 		}
 		else
 		{
-			char	   *p;
+			char	   *p = s;
 
 			if (hi_surrogate != -1)
 return JSON_UNICODE_LOW_SURROGATE;
@@ -853,7 +854,13 @@ json_lex_string(JsonLexContext *lex)
 			 * Skip to the first byte that requires special handling, so we
 			 * can batch calls to appendBinaryStringInfo.
 			 */
-			for (p = s; p < end; p++)
+			while (p < end - sizeof(Vector) &&
+   !pg_lfind8('\\', (uint8 *) p, sizeof(Vector)) &&
+   !pg_lfind8('"', (uint8 *) p, sizeof(Vector)) &&
+   !pg_lfind8_le(0x1F, (uint8 *) p, sizeof(Vector)))
+p += sizeof(Vector);
+
+			for (; p < end; p++)
 			{
 if (*p == '\\' || *p == '"')
 	break;
diff --git a/src/include/port/pg_lfind.h b/src/include/port/pg_lfind.h
index fb125977b2..bb4033c7fc 100644
--- a/src/include/port/pg_lfind.h
+++ b/src/include/port/pg_lfind.h
@@ -1,7 +1,7 @@
 /*-
  *
  * pg_lfind.h
- *	  Optimized linear search routines.
+ *	  Optimized linear search routines using SIMD intrinsics where available
  *
  * Copyright (c) 2022, PostgreSQL Global Development Group
  *
@@ -15,6 +15,68 @@
 
 #include "port/simd.h"
 
+/*
+ * pg_lfind8
+ *
+ * Return true if there is an element in 'base' that equals 'key', otherwise
+ * return false.
+ */
+static inline bool
+pg_lfind8(uint8 key, uint8 *base, uint32 nelem)
+{
+	uint32		i;
+	/* round down to multiple of vector length */
+	uint32		tail_idx = nelem & ~(sizeof(Vector) - 1);
+	Vector		chunk;
+
+	for (i = 0; i < tail_idx; i += sizeof(Vector))
+	{
+		vector_load(, [i]);
+		if (vector_eq_byte(chunk, key))
+			return true;
+	}
+
+	/* Process the remaining elements one at a time. */
+	for (; i < nelem; i++)
+	{
+		if (key == base[i])
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * pg_lfind8_le
+ *
+ * Return true if there is an element in 'base' that is less than or equal to
+ * 'key', otherwise return false.
+ */
+static inline bool
+pg_lfind8_le(uint8 key, uint8 *base, uint32 nelem)
+{
+	uint32		i;
+	/* round down to multiple of vector length */
+	uint32		tail_idx = nelem & ~(sizeof(Vector) - 1);
+	Vector		chunk;
+
+	for (i = 0; i < tail_idx; i += sizeof(Vector))
+	{
+		vector_load(, [i]);
+		if (vector_le_byte(chunk, key))
+			return true;
+	}
+
+	/* Process the remaining elements one at a time. */
+	for (; i < nelem; i++)
+	{
+		if (base[i] <= key)
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * pg_lfind32
  *
@@ -26,7 +88,6 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem)
 {
 	uint32		i = 0;
 
-	/* Use SIMD intrinsics where available. */
 #ifdef USE_SSE2
 
 	/*
diff --git a/src/include/port/simd.h b/src/include/port/simd.h
index a571e79f57..56da8e27cd 100644
--- a/src/include/port/simd.h
+++ b/src/include/port/simd.h
@@ -25,6 +25,141 @@
 #if (defined(__x86_64__) || defined(_M_AMD64))
 #include 
 #define USE_SSE2
+typedef __m128i Vector;
+
+#else
+typedef uint64 Vector;
+#endif			/* (defined(__x86_64__) || defined(_M_AMD64)) */
+
+
+static inline void vector_load(Vector * v, const uint8 *s);
+static inline Vector vector_broadcast(const uint8 c);
+static inline bool vector_has_zero(const Vector v);
+static inline bool vector_le_byte(const Vector v, const uint8 c);
+static inline bool vector_eq_byte(const Vector v, const uint8 c);
+
+
+/* load a chunk of memory into a register */
+static inline void
+vector_load(Vector * v, const uint8 *s)
+{
+#ifdef USE_SSE2
+	*v = _mm_loadu_si128((const __m128i *) s);
+#else
+	memcpy(v, s, sizeof(Vector));
+#endif
+}
+
+/* return a vector with all bytes set to c */
+static inline Vector
+vector_broadcast(const uint8 c)
+{
+#ifdef USE_SSE2
+	return _mm_set1_epi8(c);
+#else
+	return ~UINT64CONST(0) / 0xFF * c;
 #endif
+}
+
+/* return true if any bytes in the vector are zero */
+static inline bool

Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory

[mahendrakar s]

Hi Bharath,
I reviewed your patch. Minor comments.

1. Why are we not using durable_unlink instead of unlink to remove the
partial tmp files?

2. Below could be a simple if(shouldcreatetempfile){} else{} as in error
case we need to return NULL.
+ if (errno != ENOENT || !shouldcreatetempfile)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ else if (shouldcreatetempfile)
+ {
+ /*
+ * Actual file doesn't exist. Now, create a temporary file pad it
+ * and rename to the target file. The temporary file may exist from
+ * the last failed attempt (failed during partial padding or
+ * renaming or some other). If it exists, let's play safe and
+ * delete it before creating a new one.
+ */
+ snprintf(tmpsuffixpath, MAXPGPATH, "%s.tmp", targetpath);
+
+ if (dir_existsfile(tmpsuffixpath))
+ {
+ if (unlink(tmpsuffixpath) != 0)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ }
+
+ fd = open(tmpsuffixpath, flags | O_CREAT, pg_file_create_mode);
+ if (fd < 0)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ }


On Mon, 8 Aug 2022 at 11:59, Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Thu, Aug 4, 2022 at 11:59 AM Bharath Rupireddy
>  wrote:
> >
> > On Sun, Jul 31, 2022 at 8:36 PM mahendrakar s
> >  wrote:
> > >
> > >> On Mon, 25 Jul 2022 at 16:42, Bharath Rupireddy <
> bharath.rupireddyforpostg...@gmail.com> wrote:
> > >> Here's the v3 patch after rebasing.
> > >>
> > >> I just would like to reiterate the issue the patch is trying to solve:
> > >> At times (when no space  is left on the device or when the process is
> > >> taken down forcibly (VM/container crash)), there can be leftover
> > >> uninitialized .partial files (note that padding i.e. filling 16MB WAL
> > >> files with all zeros is done in non-compression cases) due to which
> > >> pg_receivewal fails to come up after the crash. To address this, the
> > >> proposed patch makes the padding 16MB WAL files atomic (first write a
> > >> .temp file, pad it and durably rename it to the original .partial
> > >> file, ready to be filled with received WAL records). This approach is
> > >> similar to what core postgres achieves atomicity while creating new
> > >> WAL file (see how XLogFileInitInternal() creates xlogtemp.%d file
> > >> first and then how InstallXLogFileSegment() durably renames it to
> > >> original WAL file).
> > >>
> > >> Thoughts?
> > >
> > > Hi Bharath,
> > >
> > > Idea to atomically allocate WAL file by creating tmp file and renaming
> it is nice.
> >
> > Thanks for reviewing it.
> >
> > > I have one question though:
> > > How is partially temp file created will be cleaned if the VM crashes
> or out of disk space cases?  Does it endup creating multiple files for
> every VM crash/disk space during process of pg_receivewal?
> > >
> > > Thoughts?
> >
> > It is handled in the patch, see [1].
> >
> > Attaching v4 patch herewith which now uses the temporary file suffix
> > '.tmp' as opposed to v3 patch '.temp'. This is just to be in sync with
> > other atomic file write codes in the core - autoprewarm,
> > pg_stat_statement, slot, basebacup, replorigin, snapbuild, receivelog
> > and so on.
> >
> > Please review the v4 patch.
>
> I've done some more testing today (hacked the code a bit by adding
> pg_usleep(1L); in pre-padding loop and crashing the pg_receivewal
> process to produce the warning [1]) and found that the better place to
> remove ".partial.tmp" leftover files is in FindStreamingStart()
> because there we do a traversal of all the files in target directory
> along the way to remove if ".partial.tmp" file(s) is/are found.
>
> Please review the v5 patch further.
>
> [1] pg_receivewal: warning: segment file
> "0001000600B9.partial" has incorrect size 15884288,
> skipping
>
> --
> Bharath Rupireddy
> RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
>

Re: [PATCH] Compression dictionaries for JSONB

[Nikita Malakhov]

Hi hackers!

I've got a partly question, partly proposal for the future development of
this
feature:
What if we use pg_dict table not to store dictionaries but to store
dictionaries'
meta, and actual dictionaries to be stored in separate tables like it is
done with
TOAST tables (i.e. pg_dict. --> pg_dict_16385 table)?
Thus we can kill several birds with one stone - we deal with concurrent
dictionaries' updates - which looks like very serious issue for now, they
do not
affect each other and overall DB performance while using, we get around SQL
statement size restriction, could effectively deal with versions in
dictionaries
and even dictionaries' versions, as well as dictionary size restriction, we
can
use it for duplicated JSON parts, and even we can provide an API to work
with dictionaries and dictionary tables which later could be usable even
for
working with JSON schemas as well (maybe, with some extension)?

Overall structure could look like this:
pg_dict
   |
   | dictionary 1 meta
   |   |--name
   |   |--size
   |   |--etc
   |   |--dictionary table name (i.e. pg_dict_16385)
   |  |
   |  |> pg_dict_16385
   |
   | dictionary 2 meta
   |   |--name
   |   |--size
   |   |--etc
   |   |--dictionary table name (i.e. pg_dict_16386)
   |  |
   |  |> pg_dict_16386
  ...

where dictionary table could look like
pg_dict_16385
   |
   | key 1
   ||-value
   |
   | key 2
   ||-value
  ...

And with a special DICT API we would have means to access, cache, store our
dictionaries in a uniform way from different levels. In this implementation
it also
looks as a very valuable addition for our JSONb Toaster.

JSON schema processing is a very promising feature and we have to keep up
with major competitors like Oracle which are already working on it.

On Mon, Aug 1, 2022 at 2:25 PM Aleksander Alekseev 
wrote:

> Hi hackers,
>
> > So far we seem to have a consensus to:
> >
> > 1. Use bytea instead of NameData to store dictionary entries;
> >
> > 2. Assign monotonically ascending IDs to the entries instead of using
> > Oids, as it is done with pg_class.relnatts. In order to do this we
> > should either add a corresponding column to pg_type, or add a new
> > catalog table, e.g. pg_dict_meta. Personally I don't have a strong
> > opinion on what is better. Thoughts?
> >
> > Both changes should be straightforward to implement and also are a
> > good exercise to newcomers.
> >
> > I invite anyone interested to join this effort as a co-author! (since,
> > honestly, rewriting the same feature over and over again alone is
> > quite boring :D).
>
> cfbot complained that v5 doesn't apply anymore. Here is the rebased
> version of the patch.
>
> > Good point. This was not a problem for ZSON since the dictionary size
> > was limited to 2**16 entries, the dictionary was immutable, and the
> > dictionaries had versions. For compression dictionaries we removed the
> > 2**16 entries limit and also decided to get rid of versions. The idea
> > was that you can simply continue adding new entries, but no one
> > thought about the fact that this will consume the memory required to
> > decompress the document indefinitely.
> >
> > Maybe we should return to the idea of limited dictionary size and
> > versions. Objections?
> > [ ...]
> > You are right. Another reason to return to the idea of dictionary
> versions.
>
> Since no one objected so far and/or proposed a better idea I assume
> this can be added to the list of TODOs as well.
>
> --
> Best regards,
> Aleksander Alekseev
>


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

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

[Peter Smith]

Here are some review comments for the patch v23-0004:

==

4.1 src/test/subscription/t/032_streaming_apply.pl

This test file was introduced in patch 0003, but I think there are a
few changes in this 0004 patch which are really have nothing to do
with 0004 and should have been included in the original 0003.

e.g. There are multiple comments like below - these belong back in the
0003 patch
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.
# Wait for this streaming transaction to be applied in the apply worker.

~~~

4.2

@@ -166,17 +175,6 @@ CREATE TRIGGER tri_tab1_unsafe
 BEFORE INSERT ON public.test_tab1
 FOR EACH ROW EXECUTE PROCEDURE trigger_func_tab1_unsafe();
 ALTER TABLE test_tab1 ENABLE REPLICA TRIGGER tri_tab1_unsafe;
-
-CREATE FUNCTION trigger_func_tab1_safe() RETURNS TRIGGER AS \$\$
-  BEGIN
-RAISE NOTICE 'test for safe trigger function';
- RETURN NEW;
-  END
-\$\$ language plpgsql;
-ALTER FUNCTION trigger_func_tab1_safe IMMUTABLE;
-CREATE TRIGGER tri_tab1_safe
-BEFORE INSERT ON public.test_tab1
-FOR EACH ROW EXECUTE PROCEDURE trigger_func_tab1_safe();
 });

I didn't understand why all this trigger_func_tab1_safe which was
added in patch 0003 is now getting removed in patch 0004. Maybe there
is some good reason, but it doesn't seem right to be adding code in
one patch and then removing it again in the next patch.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Strip -mmacosx-version-min options from plperl build

[Peter Eisentraut]

On 18.08.22 15:53, Tom Lane wrote:

Agreed on rejecting -mmacosx-version-min, but I wonder if we should
think about adopting a whitelist-instead-of-blacklist approach to
adopting stuff from perl_embed_ldflags.  ISTR that in pltcl we already
use the approach of accepting only -L and -l, and perhaps similar
strictness would serve us well here.

As an example, on a not-too-new MacPorts install, I see

$ /opt/local/bin/perl -MExtUtils::Embed -e ldopts
 -L/opt/local/lib -Wl,-headerpad_max_install_names   
-fstack-protector-strong  
-L/opt/local/lib/perl5/5.28/darwin-thread-multi-2level/CORE -lperl

I can't see any really good reason why we should allow perl
to be injecting that sort of -f option into the plperl build,
and I'm pretty dubious about the -headerpad_max_install_names
bit too.

I think also that this would allow us to drop the weird dance of
trying to subtract ccdlflags.


After analyzing the source code of ExtUtils::Embed's ldopts, I think we 
can also do this by subtracting $Config{ldflags}, since


my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";

and we really just want the $ld_or_bs part. (@archives should be empty 
for our uses.)


This would get rid of -mmacosx-version-min and -arch and all the things 
you showed, including -L/opt/local/lib, which is probably there so that 
the build of Perl itself could look there for things, but we don't need it.From 58e429d7c5a0b7f9dd720ee948d90c7d45de4638 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 19 Aug 2022 08:07:23 +0200
Subject: [PATCH v2] Remove further unwanted linker flags from
 perl_embed_ldflags

Remove the contents of $Config{ldflags} from ExtUtils::Embed's ldopts,
like we already do with $Config{ccdlflags}.  Those flags the choices
of those who built the Perl installation, which are not necessarily
appropriate for building PostgreSQL.  What we really want from ldopts
are the options identifying the location and name of the libperl
library, but unfortunately it doesn't appear possible to get that
separately from the other stuff.

The motivation for this was to strip -mmacosx-version-min options.  We
already did something similar for the -arch option.  Both of those are
now covered by this more general approach.
---
 config/perl.m4 | 10 +-
 configure  |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/config/perl.m4 b/config/perl.m4
index c823fc8cf0..c9fd91397c 100644
--- a/config/perl.m4
+++ b/config/perl.m4
@@ -81,9 +81,9 @@ AC_MSG_RESULT([$perl_embed_ccflags])
 # PGAC_CHECK_PERL_EMBED_LDFLAGS
 # -
 # We are after Embed's ldopts, but without the subset mentioned in
-# Config's ccdlflags; and also without any -arch flags, which recent
-# Apple releases put in unhelpfully.  (If you want a multiarch build
-# you'd better be specifying it in more places than plperl's final link.)
+# Config's ccdlflags and ldflags.  (Those are the choices of those who
+# built the Perl installation, which are not necessarily appropriate
+# for building PostgreSQL.)
 AC_DEFUN([PGAC_CHECK_PERL_EMBED_LDFLAGS],
 [AC_REQUIRE([PGAC_PATH_PERL])
 AC_MSG_CHECKING(for flags to link embedded Perl)
@@ -99,8 +99,8 @@ if test "$PORTNAME" = "win32" ; then
fi
 else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
-   pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
-   perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e 
"s%$pgac_tmp2%%" -e ["s/ -arch [-a-zA-Z0-9_]*//g"]`
+   pgac_tmp2=`$PERL -MConfig -e 'print "$Config{ccdlflags} 
$Config{ldflags}"'`
+   perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e 
"s%$pgac_tmp2%%"`
 fi
 AC_SUBST(perl_embed_ldflags)dnl
 if test -z "$perl_embed_ldflags" ; then
diff --git a/configure b/configure
index 176e0f9b00..d9b461ed91 100755
--- a/configure
+++ b/configure
@@ -10478,8 +10478,8 @@ if test "$PORTNAME" = "win32" ; then
fi
 else
pgac_tmp1=`$PERL -MExtUtils::Embed -e ldopts`
-   pgac_tmp2=`$PERL -MConfig -e 'print $Config{ccdlflags}'`
-   perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e 
"s%$pgac_tmp2%%" -e "s/ -arch [-a-zA-Z0-9_]*//g"`
+   pgac_tmp2=`$PERL -MConfig -e 'print "$Config{ccdlflags} 
$Config{ldflags}"'`
+   perl_embed_ldflags=`echo X"$pgac_tmp1" | sed -e "s/^X//" -e 
"s%$pgac_tmp2%%"`
 fi
 if test -z "$perl_embed_ldflags" ; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
-- 
2.37.1

Re: shared-memory based stats collector - v70

[Drouvot, Bertrand]

Hi,

On 8/18/22 9:51 PM, Andres Freund wrote:

Hi,

On 2022-08-18 15:26:31 -0400, Greg Stark wrote:

And indexes of course. It's a bit frustrating since without the
catalog you won't know what table the index actually is for... But
they're pretty important stats.

FWIW, I think we should split relation stats into table and index
stats. Historically it'd have added a lot of complexity to separate the two,
but I don't think that's the case anymore. And we waste space for index stats
by having lots of table specific fields.


It seems to me that we should work on that first then, what do you 
think? (If so I can try to have a look at it).


And once done then resume the work to provide the APIs to get all 
tables/indexes from all the databases.


That way we'll be able to provide one API for the tables and one for the 
indexes (instead of one API for both like my current POC is doing).



On that note though... What do you think about having the capability
to add other stats kinds to the stats infrastructure?


I think that's a good idea and that would be great to have.


Getting closer to that was one of my goals working on the shared memory stats
stuff.



It would make a lot of sense for pg_stat_statements to add its entries here
instead of having to reimplement a lot of the same magic.

Yes, we should move pg_stat_statements over.

It's pretty easy to get massive contention on stats entries with
pg_stat_statements, because it doesn't have support for "batching" updates to
shared stats. And reimplementing the same logic in pg_stat_statements.c
doesn't make sense.

And the set of normalized queries could probably stored in DSA as well - the
file based thing we have right now is problematic.



To do that I guess more of the code needs to be moved to be table
driven from the kind structs either with callbacks or with other meta
data.

Pretty much all of it already is. The only substantial missing bit is
reading/writing of stats files, but that should be pretty easy. And of course
making the callback array extensible.



So the kind record could contain tupledesc and the code to construct the
returned tuple so that these functions could return any custom entry as well
as the standard entries.

I don't see how this would work well - we don't have functions returning
variable kinds of tuples. And what would convert a struct to a tuple?

Nor do I think it's needed - if you have an extension providing a new stats
kind it can also provide accessors.


I think the same (the extension should be able to do that).

I really like the idea of being able to provide new stats kind.

Regards,

--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com