Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Dilip Kumar]

On Sun, Mar 21, 2021 at 9:10 AM Dilip Kumar  wrote:
>
> On Sun, Mar 21, 2021 at 7:03 AM Tom Lane  wrote:
> >
> > BTW, I tried doing "make installcheck" after having adjusted
> > default_toast_compression to be "lz4".  The compression test
> > itself fails because it's expecting the other setting; that
> > ought to be made more robust.
>
> Yeah, we need to set the default_toast_compression in the beginning of
> the test as attached.

In the last patch, I did not adjust the compression_1.out so fixed
that in the attached patch.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


v1-0001-fix-compression-test.patch
Description: Binary data

Re: pspg pager is finished

[Pavel Stehule]

ne 21. 3. 2021 v 0:48 odesílatel Thomas Munro 
napsal:

> On Sat, Mar 20, 2021 at 4:35 PM Pavel Stehule 
> wrote:
> > I finished work on pspg.
>
> tmunro@x1:~/projects/postgresql$ pspg --stream --graph
> pspg: unrecognized option '--graph'
> Try pspg --help
>
> Hmm, seems to be not finished :-)
>

:-)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hi Zhihing,
I think @Andreas ment to mark it as a todo to cleanup later.

On Sun, 21 Mar 2021 at 4:49 AM Zhihong Yu  wrote:

> Hi,
> In v11-0004-fk_arrays_elems_edits.patch :
>
> +   riinfo->fk_reftypes[i] ==
> FKCONSTR_REF_EACH_ELEMENT ? OID_ARRAY_ELEMCONTAINED_OP :
> riinfo->pf_eq_oprs[i], // XXX
>
> Is XXX placeholder for some comment you will fill in later ?
>
> Cheers
>
> On Sat, Mar 20, 2021 at 11:42 AM Mark Rofail 
> wrote:
>
>> Hey Andreas and Joel!
>>
>> Thank you so much for your hard work!
>>
>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>>> anti-join instead. The code is much clearer now IMHO.
>>>
>> It is much cleaner! I
>>
>> 2. That instead of selecting operators at execution time we save which
>>> operators to use in pg_constraint.
>>
>> This is a clever approach. If you have any updates on this please let me
>> know.
>>
>> I am still reviewing your changes. I have split your changes from my work
>> to be able to isolate the changes and review them carefully. And to help
>> others review the changes.
>>
>> Changelist:
>> - v11 (compatible with current master 2021, 03, 20,
>> commit e835e89a0fd267871e7fbddc39ad00ee3d0cb55c)
>> * rebase
>> * split andreas and joel's work
>>
>>
>> On Tue, Mar 16, 2021 at 1:27 AM Andreas Karlsson 
>> wrote:
>>
>>> On 3/15/21 4:29 PM, Mark Rofail wrote:
>>> > Actually, your fix led me in the right way, the issue was how windows
>>> > handle pointers.
>>>
>>> Hi,
>>>
>>> I have started working on a partially new strategy for the second patch.
>>> The ideas are:
>>>
>>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>>> anti-join instead (this was implemented by Joel Jacobson with cleanup
>>> and finishing touches by me). The code is much clearer now IMHO.
>>>
>>> 2. That instead of selecting operators at execution time we save which
>>> operators to use in pg_constraint. This is heavily a work in progress in
>>> my attached patches. I am not 100% convinced this is the right way to go
>>> but I plan to work some on this this weekend to see how ti will work out.
>>>
>>> Another thing I will look into is you gin patch. While you fixed it for
>>> simple scalar types which fit into the Datum type I wonder if we do not
>>> also need to copy types which are too large to fit into a Datum but I
>>> have not investigated yet which memory context the datum passed to
>>> ginqueryarrayextract() is allocated in.
>>>
>>> Andreas
>>>
>>>

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Dilip Kumar]

On Sun, Mar 21, 2021 at 7:03 AM Tom Lane  wrote:
>
> BTW, I tried doing "make installcheck" after having adjusted
> default_toast_compression to be "lz4".  The compression test
> itself fails because it's expecting the other setting; that
> ought to be made more robust.

Yeah, we need to set the default_toast_compression in the beginning of
the test as attached.

  Also, I see some diffs in the
> indirect_toast test, which seems perhaps worthy of investigation.
> (The diffs look to be just row ordering, but why?)

I will look into this.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-fix-compression-test.patch
Description: Binary data

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Zhihong Yu]

Hi,
In v11-0004-fk_arrays_elems_edits.patch :

+   riinfo->fk_reftypes[i] ==
FKCONSTR_REF_EACH_ELEMENT ? OID_ARRAY_ELEMCONTAINED_OP :
riinfo->pf_eq_oprs[i], // XXX

Is XXX placeholder for some comment you will fill in later ?

Cheers

On Sat, Mar 20, 2021 at 11:42 AM Mark Rofail  wrote:

> Hey Andreas and Joel!
>
> Thank you so much for your hard work!
>
> 1. I have removed the dependency on count(DISTINCT ...) by using an
>> anti-join instead. The code is much clearer now IMHO.
>>
> It is much cleaner! I
>
> 2. That instead of selecting operators at execution time we save which
>> operators to use in pg_constraint.
>
> This is a clever approach. If you have any updates on this please let me
> know.
>
> I am still reviewing your changes. I have split your changes from my work
> to be able to isolate the changes and review them carefully. And to help
> others review the changes.
>
> Changelist:
> - v11 (compatible with current master 2021, 03, 20,
> commit e835e89a0fd267871e7fbddc39ad00ee3d0cb55c)
> * rebase
> * split andreas and joel's work
>
>
> On Tue, Mar 16, 2021 at 1:27 AM Andreas Karlsson 
> wrote:
>
>> On 3/15/21 4:29 PM, Mark Rofail wrote:
>> > Actually, your fix led me in the right way, the issue was how windows
>> > handle pointers.
>>
>> Hi,
>>
>> I have started working on a partially new strategy for the second patch.
>> The ideas are:
>>
>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>> anti-join instead (this was implemented by Joel Jacobson with cleanup
>> and finishing touches by me). The code is much clearer now IMHO.
>>
>> 2. That instead of selecting operators at execution time we save which
>> operators to use in pg_constraint. This is heavily a work in progress in
>> my attached patches. I am not 100% convinced this is the right way to go
>> but I plan to work some on this this weekend to see how ti will work out.
>>
>> Another thing I will look into is you gin patch. While you fixed it for
>> simple scalar types which fit into the Datum type I wonder if we do not
>> also need to copy types which are too large to fit into a Datum but I
>> have not investigated yet which memory context the datum passed to
>> ginqueryarrayextract() is allocated in.
>>
>> Andreas
>>
>>

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Tom Lane]

BTW, I tried doing "make installcheck" after having adjusted
default_toast_compression to be "lz4".  The compression test
itself fails because it's expecting the other setting; that
ought to be made more robust.  Also, I see some diffs in the
indirect_toast test, which seems perhaps worthy of investigation.
(The diffs look to be just row ordering, but why?)

regards, tom lane

Re: popcount

[David Fetter]

On Sat, Mar 20, 2021 at 09:01:25PM +0100, Peter Eisentraut wrote:
> On 18.03.21 13:51, John Naylor wrote:
> > Hi David,
> > 
> > Just a nitpick:
> > 
> > +SET bytea_output TO hex;
> > 
> > Since we don't see the string in the output, I don't immediately see the
> > reason to change the output format here?

That's how I got it to work. If there's a way to make it go without
that, I'd be delighted to learn what it is :)

> > Aside from that, this patch works as expected, and is ready for committer.
> 
> I have now read the entire internet on what a suitable name for this
> function could be.  I think the emerging winner is BIT_COUNT(), which
> already exists in MySQL, and also in Python (int.bit_count()) and Java
> (Integer.bitCount()).

Thanks for doing this tedious work. Please find attached the next
version of the patch.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 9483d41941b5daa44e46e6fb164846647d716406 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 30 Dec 2020 02:51:46 -0800
Subject: [PATCH v5] popcount
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.30.2"

This is a multi-part message in MIME format.
--2.30.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Now it's accessible to SQL for the BIT VARYING and BYTEA types.

diff --git doc/src/sgml/func.sgml doc/src/sgml/func.sgml
index 68fe6a95b4..066431fd3c 100644
--- doc/src/sgml/func.sgml
+++ doc/src/sgml/func.sgml
@@ -4030,6 +4030,23 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');
   
  
 
+  
+   
+
+ bit_count
+
+bit_count ( bytes bytea )
+bigint
+   
+   
+Counts the number of bits set in a binary string.
+   
+   
+bit_count('\xdeadbeef'::bytea)
+24
+   
+  
+
   

 
@@ -4830,6 +4847,23 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');

   
 
+  
+   
+
+ bit_count
+
+bit_count ( bits bit )
+bigint
+   
+   
+Counts the bits set in a bit string.
+   
+   
+bit_count(B'101010101010101010')
+9
+   
+  
+
   

 
@@ -4869,6 +4903,7 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');
 101010001010101010

   
+
  
 

diff --git src/include/catalog/pg_proc.dat src/include/catalog/pg_proc.dat
index e259531f60..feb00eccf9 100644
--- src/include/catalog/pg_proc.dat
+++ src/include/catalog/pg_proc.dat
@@ -1446,6 +1446,9 @@
 { oid => '752', descr => 'substitute portion of string',
   proname => 'overlay', prorettype => 'bytea',
   proargtypes => 'bytea bytea int4', prosrc => 'byteaoverlay_no_len' },
+{ oid => '8436', descr => 'count set bits',
+  proname => 'bit_count', prorettype => 'int8', proargtypes => 'bytea',
+  prosrc => 'byteapopcount'},
 
 { oid => '725',
   proname => 'dist_pl', prorettype => 'float8', proargtypes => 'point line',
@@ -3876,6 +3879,9 @@
 { oid => '3033', descr => 'set bit',
   proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
   prosrc => 'bitsetbit' },
+{ oid => '8435', descr => 'count set bits',
+  proname => 'bit_count', prorettype => 'int8', proargtypes => 'bit',
+  prosrc => 'bitpopcount'},
 
 # for macaddr type support
 { oid => '436', descr => 'I/O',
diff --git src/backend/utils/adt/varbit.c src/backend/utils/adt/varbit.c
index 2235866244..c9c6c73422 100644
--- src/backend/utils/adt/varbit.c
+++ src/backend/utils/adt/varbit.c
@@ -36,6 +36,7 @@
 #include "libpq/pqformat.h"
 #include "nodes/nodeFuncs.h"
 #include "nodes/supportnodes.h"
+#include "port/pg_bitutils.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/varbit.h"
@@ -1878,3 +1879,30 @@ bitgetbit(PG_FUNCTION_ARGS)
 	else
 		PG_RETURN_INT32(0);
 }
+
+/*
+ * bitpopcount
+ *
+ * Returns the number of bits set in a bit string.
+ *
+ */
+Datum
+bitpopcount(PG_FUNCTION_ARGS)
+{
+	/* There's really no chance of an overflow here because
+	 * to get to INT64_MAX set bits, an object would have to be
+	 * an exbibyte long, exceeding what PostgreSQL can currently
+	 * store by a factor of 2^28
+	 */
+	int64		popcount;
+	VarBit		*arg1 = PG_GETARG_VARBIT_P(0);
+	bits8		*p;
+	int			len;
+
+	p = VARBITS(arg1);
+	len = (VARBITLEN(arg1) + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
+
+	popcount = pg_popcount((char *)p, len);
+
+	PG_RETURN_INT64(popcount);
+}
diff --git src/backend/utils/adt/varlena.c src/backend/utils/adt/varlena.c
index 0bc345aa4d..95091887a9 100644
--- src/backend/utils/adt/varlena.c
+++ src/backend/utils/adt/varlena.c
@@ -3440,6 +3440,22 @@ bytea_overlay(bytea *t1, bytea *t2, int sp, int sl)
 	return result;
 }
 
+/*
+ * popcount
+ */
+Datum
+byteapopcount(PG_FUNCTION_ARGS)
+{
+	

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Tom Lane]

Justin Pryzby  writes:
> On Sat, Mar 20, 2021 at 05:37:07PM -0400, Tom Lane wrote:
>> Digging around, it looks like the "-I/opt/local/include" bit came
>> from LZ4_CFLAGS, which we then stuck into CFLAGS, but it needed
>> to be put in CPPFLAGS in order to make this test work.

> If it's the same as the issue Andrey reported, then it causes a ./configure
> WARNING, which is resolved by the ac_save hack, which I copied from ICU.

I think probably what we need to do, rather than shove the pkg-config
results willy-nilly into our flags, is to disassemble them like we do
with the same results for xml2.  If you ask me, the way we are handling
ICU flags is a poor precedent that is going to blow up at some point;
the only reason it hasn't is that people aren't building --with-icu that
much yet.

regards, tom lane

Re: pspg pager is finished

[Thomas Munro]

On Sat, Mar 20, 2021 at 4:35 PM Pavel Stehule  wrote:
> I finished work on pspg.

tmunro@x1:~/projects/postgresql$ pspg --stream --graph
pspg: unrecognized option '--graph'
Try pspg --help

Hmm, seems to be not finished :-)

Re: proposal - psql - use pager for \watch command

[Thomas Munro]

On Sun, Mar 21, 2021 at 11:44 AM Thomas Munro  wrote:
> [review]

Oh, just BTW, to save confusion for others who might try this:  It
seems there is something wrong with pspg --stream on macOS, at least
when using MacPorts.  I assumed it might be just pspg 3.1.5 being too
old (that's what MacPorts has currently), so I didn't mention it
before, but I just built pspg from your github master branch and it
has the same symptom.  It doesn't seem to repaint the screen until you
press a key.  I can see that psql is doing its job, but pspg is
sitting in select() reached from ncurses wgetch():

  * frame #0: 0x00019b4af0e8 libsystem_kernel.dylib`__select + 8
frame #1: 0x000100ca0620 libncurses.6.dylib`_nc_timed_wait + 332
frame #2: 0x000100c85444 libncurses.6.dylib`_nc_wgetch + 296
frame #3: 0x000100c85b24 libncurses.6.dylib`wgetch + 52
frame #4: 0x000100a815e4 pspg`get_event + 624
frame #5: 0x000100a7899c pspg`main + 9640
frame #6: 0x00019b4f9f34 libdyld.dylib`start + 4

That's using MacPorts' libncurses.  I couldn't get it to build against
Apple's libcurses (some missing functions).  It's the same for both
your V2 and the fixup I posted.  When you press a key, it suddenly
catches up and repaints all the \watch updates that were buffered.

It works fine on Linux and FreeBSD though (I tried pspg 4.1.0 from
Debian's package manager, and pspg 4.3.1 from FreeBSD's).

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

On 3/20/21 4:21 PM, Justin Pryzby wrote:
> On Sat, Mar 20, 2021 at 04:13:47PM +0100, Tomas Vondra wrote:
>> +++ b/src/backend/access/brin/brin_tuple.c
>> @@ -213,10 +213,20 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, 
>> BrinMemTuple *tuple,
>>  (atttype->typstorage == TYPSTORAGE_EXTENDED ||
>>   atttype->typstorage == TYPSTORAGE_MAIN))
>>  {
>> +Datum   cvalue;
>> +charcompression = 
>> GetDefaultToastCompression();
>>  Form_pg_attribute att = 
>> TupleDescAttr(brdesc->bd_tupdesc,
>>  
>>   keyno);
>> -Datum   cvalue = 
>> toast_compress_datum(value,
>> -
>>   att->attcompression);
>> +
>> +/*
>> + * If the BRIN summary and indexed attribute 
>> use the same data
>> + * type, we can the same compression method. 
>> Otherwise we have
> 
> can *use ?
> 
>> + * to use the default method.
>> + */
>> +if (att->atttypid == atttype->type_id)
>> +compression = att->attcompression;
> 
> It would be more obvious to me if this said here: 
> | else: compression = GetDefaultToastCompression
> 

Thanks. I've pushed a patch tweaked per your feedback.


regards

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

Re: Using COPY FREEZE in pgbench

[Tatsuo Ishii]

Hi Fabien,

> Hello Tatsuo-san,
> 
>>> I have looked in the code of PQprotocolVersion. The only case in which
>>> it returns 0 is there's no connection. Yes, you are right. Once the
>>> connection has been successfuly established, there's no chance it
>>> fails. So I agree with you.
>>
>> Attached v3 patch addresses this.
>>
 The "g" item in the section describing initialization steps
 (i.e. option -I). I'd suggest just to replace "COPY" with "COPY
 FREEZE" in the sentence.
>>>
>>> Ok. The section is needed to be modified.
>>
>> This is also addressed in the patch.
> 
> V3 works for me and looks ok. I changed it to ready in the CF app.

Thank you for your review!
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

Rebased on HEAD.
0005 forgot to update compression_1.out.
Included changes to ./configure.ac and some other patches, but not Tomas's,
since it'll make CFBOT get mad as soon as that's pushed.

-- 
Justin
>From bf1336d284792b29e30b42af2ec820bb0c256916 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 19:12:53 -0500
Subject: [PATCH v2 01/10] Add docs for default_toast_compression..

bbe0a81db69bd10bd166907c3701492a29aca294
---
 doc/src/sgml/config.sgml | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ee4925d6d9..5cb851a5eb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8151,6 +8151,29 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
  
 
+ 
+  default_toast_compression (string)
+  
+   default_toast_compression configuration parameter
+  
+  
+  
+   
+This parameter specifies the default compression method to use
+when compressing data in TOAST tables.
+It applies only to variable-width data types.
+It may be overriden by compression clauses in the
+CREATE command, or changed after the relation is
+created by ALTER TABLE ... SET COMPRESSION.
+
+The supported compression methods are pglz and
+(if configured at the time PostgreSQL was
+built) lz4.
+The default is pglz.
+   
+  
+ 
+
  
   temp_tablespaces (string)
   
-- 
2.17.0

>From 3184c87f129ec935fa46773728869c7c6af88025 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 21:52:28 -0500
Subject: [PATCH v2 02/10] doc: pg_dump --no-toast-compression

---
 doc/src/sgml/ref/pg_dump.sgml | 12 
 1 file changed, 12 insertions(+)

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bcbb7a25fb..4a521186fb 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -931,6 +931,18 @@ PostgreSQL documentation
   
  
 
+ 
+  --no-toast-compression
+  
+   
+Do not output commands to set TOASTcompression
+methods.
+With this option, all objects will be created using whichever
+compression method is the default during restore.
+   
+  
+ 
+
  
   --no-unlogged-table-data
   
-- 
2.17.0

>From 2212020a50478c66a830f15b23c3410e4d7bb501 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 20:23:40 -0500
Subject: [PATCH v2 03/10] Compression method is an char not an OID

---
 src/backend/commands/tablecmds.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9b2800bf5e..8e756e59d5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7847,6 +7847,7 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 		index_close(indrel, lockmode);
 	}
 }
+
 /*
  * ALTER TABLE ALTER COLUMN SET STORAGE
  *
@@ -15070,7 +15071,7 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	AttrNumber	attnum;
 	char	   *compression;
 	char		typstorage;
-	Oid			cmoid;
+	char		cmethod;
 	Datum		values[Natts_pg_attribute];
 	bool		nulls[Natts_pg_attribute];
 	bool		replace[Natts_pg_attribute];
@@ -15111,9 +15112,9 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	memset(replace, false, sizeof(replace));
 
 	/* get the attribute compression method. */
-	cmoid = GetAttributeCompression(atttableform, compression);
+	cmethod = GetAttributeCompression(atttableform, compression);
 
-	atttableform->attcompression = cmoid;
+	atttableform->attcompression = cmethod;
 	CatalogTupleUpdate(attrel, >t_self, tuple);
 
 	InvokeObjectPostAlterHook(RelationRelationId,
@@ -15123,7 +15124,7 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	ReleaseSysCache(tuple);
 
 	/* apply changes to the index column as well */
-	SetIndexStorageProperties(rel, attrel, attnum, cmoid, '\0', lockmode);
+	SetIndexStorageProperties(rel, attrel, attnum, cmethod, '\0', lockmode);
 	table_close(attrel, RowExclusiveLock);
 
 	/* make changes visible */
-- 
2.17.0

>From d73643d7569ef74b9008d232f8c0ea33cb96ff3c Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 20:07:31 -0500
Subject: [PATCH v2 04/10] Remove duplicative macro

---
 src/include/access/toast_compression.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/include/access/toast_compression.h b/src/include/access/toast_compression.h
index 514df0bed1..09af6e7810 100644
--- a/src/include/access/toast_compression.h
+++ b/src/include/access/toast_compression.h
@@ -50,8 +50,6 @@ typedef enum ToastCompressionId
 			 errdetail("This functionality requires the server to be built with lz4 support."), \
 			 errhint("You need to rebuild PostgreSQL using --with-lz4.")))
 
-#define IsValidCompression(cm)  ((cm) != InvalidCompressionMethod)
-
 #define 

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Justin Pryzby]

On Sat, Mar 20, 2021 at 05:37:07PM -0400, Tom Lane wrote:
> Justin Pryzby  writes:
> > On Fri, Mar 19, 2021 at 02:07:31PM -0400, Robert Haas wrote:
> >> On Fri, Mar 19, 2021 at 1:44 PM Justin Pryzby  wrote:
> >>> configure: WARNING: lz4.h: accepted by the compiler, rejected by the 
> >>> preprocessor!
> >>> configure: WARNING: lz4.h: proceeding with the compiler's result
> 
> >> No, I don't see this. I wonder whether this could possibly be an
> >> installation issue on Andrey's machine? If not, it must be
> >> version-dependent or installation-dependent in some way.
> 
> > Andrey, can you check if latest HEAD (bbe0a81db) has these ./configure 
> > warnings ?
> 
> FWIW, I also saw that, when building HEAD against MacPorts' version
> of liblz4 on an M1 Mac.  config.log has
...
> Digging around, it looks like the "-I/opt/local/include" bit came
> from LZ4_CFLAGS, which we then stuck into CFLAGS, but it needed
> to be put in CPPFLAGS in order to make this test work.

If it's the same as the issue Andrey reported, then it causes a ./configure
WARNING, which is resolved by the ac_save hack, which I copied from ICU.

I'll shortly send a patchset including my tentative fix for that.

The configure.ac bits are also on this other thread:
https://www.postgresql.org/message-id/20210315180918.GW29463%40telsasoft.com
0005-re-add-wal_compression_method-lz4.patch 

+if test "$with_lz4" = yes; then
+  ac_save_CPPFLAGS=$CPPFLAGS
+  CPPFLAGS="$LZ4_CFLAGS $CPPFLAGS"
+
+  # Verify we have LZ4's header files
+  AC_CHECK_HEADERS(lz4/lz4.h, [],
+   [AC_CHECK_HEADERS(lz4.h, [], [AC_MSG_ERROR([lz4.h header file is 
required for LZ4])])])
+
+  CPPFLAGS=$ac_save_CPPFLAGS
+fi

-- 
Justin

Re: proposal - psql - use pager for \watch command

[Thomas Munro]

On Thu, Mar 4, 2021 at 11:28 PM Pavel Stehule  wrote:
> čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule  
> napsal:
>> Here is a little bit updated patch - detection of end of any child process 
>> cannot be used on WIN32.

Yeah, it's OK for me if this feature only works on Unix until the
right person for the job shows up with a patch.  If there is no pspg
on Windows, how would we even know if it works?

> second version - after some thinking, I think the pager for \watch command 
> should be controlled by option "pager" too.  When the pager is disabled on 
> psql level, then the pager will not be used for \watch too.

Makes sense.

+longs = Min(i, 100L);
+
+pg_usleep(1000L * s);
+
+/*
+ * in this moment an pager process can be only one child of
+ * psql process. There cannot be other processes. So we can
+ * detect end of any child process for fast detection of
+ * pager process.
+ *
+ * This simple detection doesn't work on WIN32, because we
+ * don't know handle of process created by _popen function.
+ * Own implementation of _popen function based on CreateProcess
+ * looks like overkill in this moment.
+ */
+if (pagerpipe)
+{
+
+intstatus;
+pid_tpid;
+
+pid = waitpid(-1, , WNOHANG);
+if (pid)
+break;
+}
+
+#endif
+
 if (cancel_pressed)
 break;

I thought a bit about what we're really trying to achieve here.  We
want to go to sleep until someone presses ^C, the pager exits, or a
certain time is reached.  Here, we're waking up 10 times per second to
check for exited child processes.  It works, but it does not spark
joy.

I thought about treating SIGCHLD the same way as we treat SIGINT: it
could use the siglongjmp() trick for a non-local exit from the signal
handler.  (Hmm... I wonder why that pre-existing code bothers to check
cancel_pressed, considering it is running with
sigint_interrupt_enabled = true so it won't even set the flag.)  It
feels clever, but then you'd still have the repeating short
pg_usleep() calls, for reasons described by commit 8c1a71d36f5.  I do
not like sleep/poll loops.  Think of the polar bears.  I need to fix
all of these, as a carbon emission offset for cfbot.

Although there are probably several ways to do this efficiently, my
first thought was: let's try sigtimedwait()!  If you block the signals
first, you have a race-free way to wait for SIGINT (^C), SIGCHLD
(pager exited) or a timeout you can specify.  I coded that up and it
worked pretty nicely, but then I tried it on macOS too and it didn't
compile -- Apple didn't implement that.  Blah.

Next I tried sigwait().  That's already used in our tree, so it should
be OK.  At first I thought that using SIGALRM to wake it up would be a
bit too ugly and I was going to give up, but then I realised that an
interval timer (one that automatically fires every X seconds) is
exactly what we want here, and we can set it up just once at the start
of do_watch() and cancel it at the end of do_watch().  With the
attached patch you get exactly one sigwait() syscall of the correct
duration per \watch cycle.

Thoughts?  I put my changes into a separate patch for clarity, but
they need some more tidying up.

I'll look at the documentation next.
From e2d6d4a7ee33defbc9eb33a8bec53bac57871922 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Sat, 20 Mar 2021 21:54:27 +1300
Subject: [PATCH v3 1/2] Support PSQL_WATCH_PAGER for psql's \watch command.

---
 doc/src/sgml/ref/psql-ref.sgml | 27 
 src/bin/psql/command.c | 81 +++---
 src/bin/psql/common.c  | 11 +++--
 src/bin/psql/common.h  |  2 +-
 src/bin/psql/help.c|  4 +-
 5 files changed, 112 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 01ec9b8b0a..5f594e5e0e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2993,6 +2993,11 @@ lo_import 152801
   (such as more) is used.
   
 
+  
+  When the ennvironment variable PSQL_WATCH_PAGER is set, then
+  the output of repeated execution of query is piped to the specified program.
+  
+
   
   When the pager option is off, the pager
   program is not used. When the pager option is
@@ -3004,6 +3009,13 @@ lo_import 152801
   without a value
   toggles pager use on and off.
   
+
+  
+  When an command \watch is executed, and environment
+  variable PSQL_WATCH_PAGER is defined, but the value of
+  the option pager is off, then the
+  pager is not used.
+  
   
   
 
@@ -4663,6 +4675,21 @@ 

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Tom Lane]

Justin Pryzby  writes:
> On Fri, Mar 19, 2021 at 02:07:31PM -0400, Robert Haas wrote:
>> On Fri, Mar 19, 2021 at 1:44 PM Justin Pryzby  wrote:
>>> configure: WARNING: lz4.h: accepted by the compiler, rejected by the 
>>> preprocessor!
>>> configure: WARNING: lz4.h: proceeding with the compiler's result

>> No, I don't see this. I wonder whether this could possibly be an
>> installation issue on Andrey's machine? If not, it must be
>> version-dependent or installation-dependent in some way.

> Andrey, can you check if latest HEAD (bbe0a81db) has these ./configure 
> warnings ?

FWIW, I also saw that, when building HEAD against MacPorts' version
of liblz4 on an M1 Mac.  config.log has

configure:13536: checking lz4.h usability
configure:13536: ccache clang -c -I/opt/local/include -Wall -Wmissing-prototype\
s -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmi\
ssing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unus\
ed-command-line-argument -g -O2 -isysroot /Applications/Xcode.app/Contents/Deve\
loper/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   conftest.c >&5
configure:13536: $? = 0
configure:13536: result: yes
configure:13536: checking lz4.h presence
configure:13536: ccache clang -E -isysroot /Applications/Xcode.app/Contents/Dev\
eloper/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   conftest.c
conftest.c:67:10: fatal error: 'lz4.h' file not found
#include 
 ^~~
1 error generated.
configure:13536: $? = 1

Digging around, it looks like the "-I/opt/local/include" bit came
from LZ4_CFLAGS, which we then stuck into CFLAGS, but it needed
to be put in CPPFLAGS in order to make this test work.

regards, tom lane

Re: Replication slot stats misgivings

[Andres Freund]

Hi,

On 2021-03-20 10:28:06 +0530, Amit Kapila wrote:
> On Sat, Mar 20, 2021 at 9:25 AM Amit Kapila  wrote:
> > This idea is worth exploring to address the complaints but what do we
> > do when we detect that the stats are from the different slot? It has
> > mixed of stats from the old and new slot. We need to probably reset it
> > after we detect that.
> >
> 
> What if the user created a slot with the same name after dropping the
> slot and it has used the same index. I think chances are less but
> still a possibility, but maybe that is okay.
> 
> > What if after some frequency (say whenever we
> > run out of indexes) we check whether the slots we are maintaining is
> > pgstat.c have some stale slot entry (entry exists but the actual slot
> > is dropped)?
> >
> 
> A similar drawback (the user created a slot with the same name after
> dropping it) exists with this as well.

pgstat_report_replslot_drop() already prevents that, no?

Greetings,

Andres Freund

Re: Replication slot stats misgivings

[Andres Freund]

Hi,

On 2021-03-20 09:25:40 +0530, Amit Kapila wrote:
> On Sat, Mar 20, 2021 at 12:22 AM Andres Freund  wrote:
> >
> > And then more generally about the feature:
> > - If a slot was used to stream out a large amount of changes (say an
> >   initial data load), but then replication is interrupted before the
> >   transaction is committed/aborted, stream_bytes will not reflect the
> >   many gigabytes of data we may have sent.
> >
> 
> We can probably update the stats each time we spilled or streamed the
> transaction data but it was not clear at that stage whether or how
> much it will be useful.

It seems like the obvious answer here is to sync stats when releasing
the slot?


> > - I seems weird that we went to the trouble of inventing replication
> >   slot stats, but then limit them to logical slots, and even there don't
> >   record the obvious things like the total amount of data sent.
> >
> 
> Won't spill_bytes and stream_bytes will give you the amount of data sent?

I don't think either tracks changes that were neither spilled nor
streamed? And if they are, they're terribly misnamed?

> >
> > I think the best way to address the more fundamental "pgstat related"
> > complaints is to change how replication slot stats are
> > "addressed". Instead of using the slots name, report stats using the
> > index in ReplicationSlotCtl->replication_slots.
> >
> > That removes the risk of running out of "replication slot stat slots":
> > If we loose a drop message, the index eventually will be reused and we
> > likely can detect that the stats were for a different slot by comparing
> > the slot name.
> >
> 
> This idea is worth exploring to address the complaints but what do we
> do when we detect that the stats are from the different slot?

I think it's pretty easy to make that bulletproof. Add a
pgstat_report_replslot_create(), and use that in
ReplicationSlotCreate(). That is called with
ReplicationSlotAllocationLock held, so it can just safely zero out stats.

I don't think:

> It has mixed of stats from the old and new slot.

Can happen in that scenario.


> > It also makes it easy to handle the issue of max_replication_slots being
> > lowered and there still being stats for a slot - we simply can skip
> > restoring that slots data, because we know the relevant slot can't exist
> > anymore. And we can make the initial pgstat_report_replslot() during
> > slot creation use a
> >
> 
> Here, your last sentence seems to be incomplete.

Oops, I was planning to suggest adding pgstat_report_replslot_create()
that zeroes out the pre-existing stats (or a parameter to
pgstat_report_replslot(), but I don't think that's better).


> > I'm wondering if we should just remove the slot name entirely from the
> > pgstat.c side of things, and have pg_stat_get_replication_slots()
> > inquire about slots by index as well and get the list of slots to report
> > stats for from slot.c infrastructure.
> >
> 
> But how will you detect in your idea that some of the stats from the
> already dropped slot?

I don't think that is possible with my sketch?

Greetings,

Andres Freund

Re: [HACKERS] Custom compression methods (mac+lz4.h)

[Justin Pryzby]

On Fri, Mar 19, 2021 at 02:07:31PM -0400, Robert Haas wrote:
> On Fri, Mar 19, 2021 at 1:44 PM Justin Pryzby  wrote:
> > Working with one of Andrey's patches on another thread, he reported offlist
> > getting this message, resolved by this patch.  Do you see this warning 
> > during
> > ./configure ?  The latest CI is of a single patch without the LZ4 stuff, so 
> > I
> > can't check its log.
> >
> > configure: WARNING: lz4.h: accepted by the compiler, rejected by the 
> > preprocessor!
> > configure: WARNING: lz4.h: proceeding with the compiler's result
> 
> No, I don't see this. I wonder whether this could possibly be an
> installation issue on Andrey's machine? If not, it must be
> version-dependent or installation-dependent in some way.

Andrey, can you check if latest HEAD (bbe0a81db) has these ./configure warnings 
?

If so, can you check if your environment is clean, specifically lz4.h - if
you've installed LZ4 from source, I have to imagine that's relevant.
Most users won't do that, but it should be a supported configuration, too.

-- 
Justin

Re: [HACKERS] Custom compression methods (./configure)

[Tom Lane]

Justin Pryzby  writes:
> On Fri, Mar 19, 2021 at 05:35:58PM -0300, Alvaro Herrera wrote:
>> I find this behavior confusing; I'd rather have configure error out if
>> it can't find the package support I requested, than continuing with a
>> set of configure options different from what I gave.

> That's clearly wrong, but that's not the behavior I see:

Yeah, it errors out as-expected for me too, on a couple of different
machines (see sifaka's latest run for documentation).

regards, tom lane

Re: [HACKERS] Custom compression methods (./configure)

[Alvaro Herrera]

On 2021-Mar-20, Justin Pryzby wrote:

> On Fri, Mar 19, 2021 at 05:35:58PM -0300, Alvaro Herrera wrote:
> > Hmm, if I use configure --with-lz4, I get this:
> > 
> > checking whether to build with LZ4 support... yes
> > checking for liblz4... no
> > configure: error: Package requirements (liblz4) were not met:
> > 
> > No package 'liblz4' found
> ...
> > See the pkg-config man page for more details.
> > running CONFIG_SHELL=/bin/bash /bin/bash /pgsql/source/master/configure 
> > --enable-debug --enable-depend --enable-cassert --enable-nls 
> > --cache-file=/home/alvherre/run/pgconfig.master.cache 
> > --enable-thread-safety --with-python --with-perl --with-tcl --with-openssl 
> > --with-libxml --enable-tap-tests --with-tclconfig=/usr/lib/tcl8.6 
> > PYTHON=/usr/bin/python3 --with-llvm --prefix=/pgsql/install/master 
> > --with-pgport=55432 --no-create --no-recursion

> I can't reproduce the behavior - is it because of your --cache-file or
> something ?

Argh, yeah, you're right -- my custom scripting was confusing the issue,
by rerunning configure automatically with the options previously in the
cache file.  I had the equivalent of "configure ; make" so when
configure failed, the make step re-ran configure using the options in
the cache file, which did not have --with-lz4.

-- 
Álvaro Herrera   Valdivia, Chile

Re: [HACKERS] Custom compression methods (./configure)

[Justin Pryzby]

On Fri, Mar 19, 2021 at 05:35:58PM -0300, Alvaro Herrera wrote:
> Hmm, if I use configure --with-lz4, I get this:
> 
>   checking whether to build with LZ4 support... yes
>   checking for liblz4... no
>   configure: error: Package requirements (liblz4) were not met:
> 
>   No package 'liblz4' found
...
>   See the pkg-config man page for more details.
>   running CONFIG_SHELL=/bin/bash /bin/bash /pgsql/source/master/configure 
> --enable-debug --enable-depend --enable-cassert --enable-nls 
> --cache-file=/home/alvherre/run/pgconfig.master.cache --enable-thread-safety 
> --with-python --with-perl --with-tcl --with-openssl --with-libxml 
> --enable-tap-tests --with-tclconfig=/usr/lib/tcl8.6 PYTHON=/usr/bin/python3 
> --with-llvm --prefix=/pgsql/install/master --with-pgport=55432 --no-create 
> --no-recursion
>   ...
> 
> I find this behavior confusing; I'd rather have configure error out if
> it can't find the package support I requested, than continuing with a
> set of configure options different from what I gave.

That's clearly wrong, but that's not the behavior I see:

|$ ./configure --with-lz4 ; echo $?
|...
|checking for liblz4... no
|configure: error: Package requirements (liblz4) were not met:
|
|No package 'liblz4' found
|
|Consider adjusting the PKG_CONFIG_PATH environment variable if you
|installed software in a non-standard prefix.
|
|Alternatively, you may set the environment variables LZ4_CFLAGS
|and LZ4_LIBS to avoid the need to call pkg-config.
|See the pkg-config man page for more details.
|1

I can't reproduce the behavior - is it because of your --cache-file or
something ?

-- 
Justin

Re: [HACKERS] Custom compression methods

[Tom Lane]

Andrew Dunstan  writes:
> On 3/20/21 3:03 PM, Tom Lane wrote:
>> I fixed up some issues in 0008/0009 (mostly cosmetic, except that
>> you forgot a server version check in dumpToastCompression) and
>> pushed that, so we can see if it makes crake happy.

> It's still produced a significant amount more difference between the
> dumps. For now I've increased the fuzz factor a bit like this:

Ah, our emails crossed.

> I'll try to come up with something better. Maybe just ignore lines like
> SET default_toast_compression = 'pglz';
> when taking the diff.

I noticed that there were a fair number of other diffs besides those.
Seems like we need some better comparison technology, really, but I'm
not certain what.

regards, tom lane

Re: [HACKERS] Custom compression methods

[Tom Lane]

I wrote:
> I fixed up some issues in 0008/0009 (mostly cosmetic, except that
> you forgot a server version check in dumpToastCompression) and
> pushed that, so we can see if it makes crake happy.

crake was still unhappy with that:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake=2021-03-20%2019%3A03%3A56

but I see it just went green ... did you do something to adjust
the expected output?

regards, tom lane

Re: [HACKERS] Custom compression methods

[Andrew Dunstan]

On 3/20/21 3:03 PM, Tom Lane wrote:
> I wrote:
>> Yeah, _doSetFixedOutputState is the wrong place: that runs on the
>> pg_restore side of the fence, and would not have access to the
>> necessary info in a separated dump/restore run.
>> It might be necessary to explicitly pass the state through in a TOC item,
>> as we do for things like the standard_conforming_strings setting.
> Ah, now that I read your patch I see that's exactly what you did.
>
> I fixed up some issues in 0008/0009 (mostly cosmetic, except that
> you forgot a server version check in dumpToastCompression) and
> pushed that, so we can see if it makes crake happy.
>
>   


It's still produced a significant amount more difference between the
dumps. For now I've increased the fuzz factor a bit like this:


diff --git a/PGBuild/Modules/TestUpgradeXversion.pm
b/PGBuild/Modules/TestUpgradeXversion.pm
index 1d1d313..567d7cb 100644
--- a/PGBuild/Modules/TestUpgradeXversion.pm
+++ b/PGBuild/Modules/TestUpgradeXversion.pm
@@ -621,7 +621,7 @@ sub test_upgrade    ## no critic
(Subroutines::ProhibitManyArgs)
    # generally from reordering of larg object output.
    # If not we heuristically allow up to 2000 lines of diffs
 
-   if (   ($oversion ne $this_branch && $difflines < 2000)
+   if (   ($oversion ne $this_branch && $difflines < 2700)
    || ($oversion eq $this_branch) && $difflines < 50)
    {
    return 1;


I'll try to come up with something better. Maybe just ignore lines like

SET default_toast_compression = 'pglz';

when taking the diff.


cheers


andrew


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

Re: popcount

[Peter Eisentraut]

On 18.03.21 13:51, John Naylor wrote:

Hi David,

Just a nitpick:

+SET bytea_output TO hex;

Since we don't see the string in the output, I don't immediately see the 
reason to change the output format here?


Aside from that, this patch works as expected, and is ready for committer.


I have now read the entire internet on what a suitable name for this 
function could be.  I think the emerging winner is BIT_COUNT(), which 
already exists in MySQL, and also in Python (int.bit_count()) and Java 
(Integer.bitCount()).

Re: [HACKERS] Custom compression methods

[Tom Lane]

I wrote:
> Yeah, _doSetFixedOutputState is the wrong place: that runs on the
> pg_restore side of the fence, and would not have access to the
> necessary info in a separated dump/restore run.

> It might be necessary to explicitly pass the state through in a TOC item,
> as we do for things like the standard_conforming_strings setting.

Ah, now that I read your patch I see that's exactly what you did.

I fixed up some issues in 0008/0009 (mostly cosmetic, except that
you forgot a server version check in dumpToastCompression) and
pushed that, so we can see if it makes crake happy.

regards, tom lane

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Sat, Mar 20, 2021 at 01:36:15PM -0400, Tom Lane wrote:
> Justin Pryzby  writes:
> > On Sat, Mar 20, 2021 at 04:37:53PM +0530, Dilip Kumar wrote:
> >> - And, 0008 and 0009, I think my
> >> 0001-Fixup-dump-toast-compression-method.patch[1] is doing this in a
> >> much simpler way, please have a look and let me know if you think that
> >> has any problems and we need to do the way you are doing here?
> 
> > I tested and saw that your patch doesn't output "SET 
> > default_toast_compression"
> > in non-text dumps (pg_dump -Fc).
> 
> Yeah, _doSetFixedOutputState is the wrong place: that runs on the
> pg_restore side of the fence, and would not have access to the
> necessary info in a separated dump/restore run.
> 
> It might be necessary to explicitly pass the state through in a TOC item,
> as we do for things like the standard_conforming_strings setting.

My patches do this in 0008 and 0009 - I'd appreciate if you'd take a look.
0009 edits parts of 0008, and if that's all correct then they should be
squished together.

https://www.postgresql.org/message-id/20210320074420.GR11765%40telsasoft.com

-- 
Justin

Re: [PATCH] proposal for regexp_count, regexp_instr, regexp_substr and regexp_replace

[Gilles Darold]

Hi,


This is a new version of the patch that now implements all the XQUERY
regexp functions as described in the standard, minus the differences of
PostgerSQL regular expression explain in [1].


The standard SQL describe functions like_regex(), occurrences_regex(),
position_regex(), substring_regex() and translate_regex() which
correspond to the commonly named functions regexp_like(),
regexp_count(), regexp_instr(), regexp_substr() and regexp_replace() as
reported by Chapman Flack in [2]. All these function are implemented in
the patch. Syntax of the functions are:


- regexp_like(string, pattern [, flags ])

- regexp_count( string, pattern [, position ] [, flags ])

- regexp_instr( string, pattern [, position ] [, occurrence ] [,
returnopt ] [, flags ] [, group ])

- regexp_substr( string, pattern [, position ] [, occurrence ] [, flags
] [, group ])

- regexp_replace(source, pattern, replacement [, position ] [,
occurrence ] [, flags ])


In addition to previous patch version I have added the regexp()_like
function and extended the existsing regex_replace() function. The patch
documents these functions and adds regression tests for all functions. I
will add it to the commitfest.


An other regexp functions regexp_positions() that returns all
occurrences that matched a POSIX regular expression is also developped
by Joel Jacobson, see [2]. This function expands the list of regexp
functions described in XQUERY.


[1]
https://www.postgresql.org/docs/13/functions-matching.html#FUNCTIONS-POSIX-REGEXP

[2]
https://www.postgresql.org/message-id/flat/bfd5-909d-408b-8531-95b32f18d4ab%40www.fastmail.com#3ec8ba658eeabcae2ac6ccca33bd1aed


-- 
Gilles Darold
LzLabs GmbH
http://www.lzlabs.com/

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fee0561961..f5f08f1509 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -3097,6 +3097,66 @@ repeat('Pg', 4) PgPgPgPg

   
 
+  
+   
+
+ regexp_count
+
+regexp_count ( string text, pattern text [, position integer ] [, flags text ] )
+integer
+   
+   
+Return the number of times a pattern occurs for a match of a POSIX
+regular expression to the string; see
+.
+   
+   
+regexp_count('123456789012', '\d{3}', 3)
+3
+   
+  
+
+  
+   
+
+ regexp_instr
+
+regexp_instr ( string text, pattern text [, position integer ] [, occurence integer ] [, returnopt integer ] [, flags text ] [, group integer ] )
+integer
+   
+   
+   Return the position within string where the
+   match of a POSIX regular expression occurs. It returns an integer
+   indicating the beginning or ending position of the matched substring,
+   depending on the value of the returnopt argument
+   (default beginning). If no match is found, then the function returns 0;
+   see .
+   
+   
+regexp_instr('1234567890', '(123)(4(56)(78))', 1, 1, 0, 'i', 4)
+7
+   
+  
+
+  
+   
+
+ regexp_like
+
+regexp_like ( string text, pattern text [, flags text ] )
+boolean
+   
+   
+Evaluate the existence of a match to a POSIX regular expression
+in string; see .
+   
+   
+regexp_like('Hello'||chr(10)||'world', '^world$', 'm')
+3
+   
+  
+
+
   

 
@@ -3144,7 +3204,7 @@ repeat('Pg', 4) PgPgPgPg
 
  regexp_replace
 
-regexp_replace ( string text, pattern text, replacement text [, flags text ] )
+regexp_replace ( string text, pattern text, replacement text [, position integer ] [, occurence integer ]  [, flags text ] )
 text


@@ -3157,6 +3217,24 @@ repeat('Pg', 4) PgPgPgPg

   
 
+  
+   
+
+ regexp_substr
+
+regexp_substr ( string text, pattern text [, position integer ] [, occurence integer ] [, flags text ] [, group integer ] )
+text
+   
+   
+   Return the substring within string corresponding to the
+   match of a POSIX regular expression; see .
+   
+   
+regexp_substr('1234567890 1234557890', '(123)(4(5[56])(78))', 1, 2, 'i', 3)
+55
+   
+  
+
   

 
@@ -5295,6 +5373,18 @@ substring('foobar' similar '#"o_b#"%' escape '#')NULL
 regexp_split_to_array

+   
+regexp_like
+   
+   
+regexp_count
+   
+   
+regexp_instr
+   
+   
+regexp_substr
+   
 

  lists the available
@@ -5451,6 +5541,8 @@ substring('foobar' from 'o(.)b')   o
  It has the syntax
  regexp_replace(source,
  pattern, replacement
+ , position 
+ , occurrence 
  , flags ).
  The source string is returned unchanged if
  there is no match to the pattern.  If there is a
@@ -5464,12 +5556,19 @@ substring('foobar' 

Re: pspg pager is finished

[dinesh kumar]

On Sat, Mar 20, 2021 at 9:05 AM Pavel Stehule 
wrote:

> Hi
>
> I finished work on pspg.
>
> https://github.com/okbob/pspg
>
> Now it has special features like rows or block selection by mouse, and
> export related data to file or to clipboard in csv or tsv or insert
> formats. Some basic features like sorting data per selected columns are
> possible too.
>
> I hope this tool will serve well, and so work with Postgres (or other
> supported databases) in the terminal will be more comfortable and more
> efficient.
>
> Regards
>
> Pavel Stehule
>

It's awesome Pavel,

Building UI at console level is a serious stuff, and I love it.
Thank you so much for all your efforts.

-- 

Regards,
Dinesh
manojadinesh.blogspot.com

Re: [HACKERS] Custom compression methods

[Tom Lane]

Justin Pryzby  writes:
> On Sat, Mar 20, 2021 at 04:37:53PM +0530, Dilip Kumar wrote:
>> - And, 0008 and 0009, I think my
>> 0001-Fixup-dump-toast-compression-method.patch[1] is doing this in a
>> much simpler way, please have a look and let me know if you think that
>> has any problems and we need to do the way you are doing here?

> I tested and saw that your patch doesn't output "SET 
> default_toast_compression"
> in non-text dumps (pg_dump -Fc).

Yeah, _doSetFixedOutputState is the wrong place: that runs on the
pg_restore side of the fence, and would not have access to the
necessary info in a separated dump/restore run.

It might be necessary to explicitly pass the state through in a TOC item,
as we do for things like the standard_conforming_strings setting.

regards, tom lane

Re: pg_upgrade failing for 200+ million Large Objects

[Jan Wieck]

On 3/20/21 11:23 AM, Tom Lane wrote:

Jan Wieck  writes:

All that aside, the entire approach doesn't scale.


Yeah, agreed.  When we gave large objects individual ownership and ACL
info, it was argued that pg_dump could afford to treat each one as a
separate TOC entry because "you wouldn't have that many of them, if
they're large".  The limits of that approach were obvious even at the
time, and I think now we're starting to see people for whom it really
doesn't work.


It actually looks more like some users have millions of "small objects". 
I am still wondering where that is coming from and why they are abusing 
LOs in that way, but that is more out of curiosity. Fact is that they 
are out there and that they cannot upgrade from their 9.5 databases, 
which are now past EOL.




I wonder if pg_dump could improve matters cheaply by aggregating the
large objects by owner and ACL contents.  That is, do

select distinct lomowner, lomacl from pg_largeobject_metadata;

and make just *one* BLOB TOC entry for each result.  Then dump out
all the matching blobs under that heading.


What I am currently experimenting with is moving the BLOB TOC entries 
into the parallel data phase of pg_restore "when doing binary upgrade". 
It seems to scale nicely with the number of cores in the system. In 
addition to that have options for pg_upgrade and pg_restore that cause 
the restore to batch them into transactions, like 10,000 objects at a 
time. There was a separate thread for that but I guess it is better to 
keep it all together here now.




A possible objection is that it'd reduce the ability to restore blobs
selectively, so maybe we'd need to make it optional.


I fully intend to make all this into new "options". I am afraid that 
there is no one-size-fits-all solution here.


Of course, that just reduces the memory consumption on the client
side; it does nothing for the locks.  Can we get away with releasing the
lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?


I'm not very fond of the idea going lockless when at the same time 
trying to parallelize the restore phase. That can lead to really nasty 
race conditions. For now I'm aiming at batches in transactions.



Regards, Jan

--
Jan Wieck
Principle Database Engineer
Amazon Web Services

Re: pg_upgrade failing for 200+ million Large Objects

[Tom Lane]

Bruce Momjian  writes:
> On Sat, Mar 20, 2021 at 11:23:19AM -0400, Tom Lane wrote:
>> Of course, that just reduces the memory consumption on the client
>> side; it does nothing for the locks.  Can we get away with releasing the
>> lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

> Well, in pg_upgrade mode you can, since there are no other cluster
> users, but you might be asking for general pg_dump usage.

Yeah, this problem doesn't only affect pg_upgrade scenarios, so it'd
really be better to find a way that isn't dependent on binary-upgrade
mode.

regards, tom lane

Re: pg_upgrade failing for 200+ million Large Objects

[Bruce Momjian]

On Sat, Mar 20, 2021 at 11:23:19AM -0400, Tom Lane wrote:
> I wonder if pg_dump could improve matters cheaply by aggregating the
> large objects by owner and ACL contents.  That is, do
> 
> select distinct lomowner, lomacl from pg_largeobject_metadata;
> 
> and make just *one* BLOB TOC entry for each result.  Then dump out
> all the matching blobs under that heading.
> 
> A possible objection is that it'd reduce the ability to restore blobs
> selectively, so maybe we'd need to make it optional.
> 
> Of course, that just reduces the memory consumption on the client
> side; it does nothing for the locks.  Can we get away with releasing the
> lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

Well, in pg_upgrade mode you can, since there are no other cluster
users, but you might be asking for general pg_dump usage.

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

  If only the physical world exists, free will is an illusion.

Re: pspg pager is finished

[Pavel Stehule]

so 20. 3. 2021 v 17:01 odesílatel Nikolay Samokhvalov 
napsal:

> On Fri, Mar 19, 2021 at 20:35 Pavel Stehule 
> wrote:
>
>> Hi
>>
>> I finished work on pspg.
>>
>> https://github.com/okbob/pspg
>>
>
> Thank you, Pavel. I use it always when possible, and highly recommend it
> to others.
>
>> 
>>
> Nik
>

Thank you

Pavel

Re: pspg pager is finished

[Nikolay Samokhvalov]

On Fri, Mar 19, 2021 at 20:35 Pavel Stehule  wrote:

> Hi
>
> I finished work on pspg.
>
> https://github.com/okbob/pspg
>

Thank you, Pavel. I use it always when possible, and highly recommend it to
others.

> 
>
Nik

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Sat, Mar 20, 2021 at 04:37:53PM +0530, Dilip Kumar wrote:
> - 0006 is fine but not sure what is the advantage over what we have today?

The advantage is that it's dozens of lines shorter, and automatically includes
a HINT.
 SET default_toast_compression = 'I do not exist compression';  

   
 ERROR:  invalid value for parameter "default_toast_compression": "I do not 
exist compression"  
   
-DETAIL:  Compression method "I do not exist compression" does not exist.   

   
+HINT:  Available values: pglz, lz4.

   

If we use a GUC hook, I think it should be to special case lz4 to say:
"..must be enabled when PG was built".

> - And, 0008 and 0009, I think my
> 0001-Fixup-dump-toast-compression-method.patch[1] is doing this in a
> much simpler way, please have a look and let me know if you think that
> has any problems and we need to do the way you are doing here?

I tested and saw that your patch doesn't output "SET default_toast_compression"
in non-text dumps (pg_dump -Fc).  Also, I think the internal newline should be
removed:

ALTER TABLE public.t ALTER COLUMN b
SET COMPRESSION lz4;

-- 
Justin

Re: Logical Replication vs. 2PC

[Amit Kapila]

On Sat, Mar 20, 2021 at 4:02 PM Dilip Kumar  wrote:
>
> On Sat, Mar 20, 2021 at 7:50 AM Amit Kapila  wrote:
> >
> > On Fri, Mar 19, 2021 at 9:22 PM Markus Wanner
> >  wrote:
>
> > So, I think you are using xid of publisher and origin_id of
> > subscription to achieve uniqueness because both will be accessible in
> > prepare and commit prepared. Right? If so, I think that will work out
> > here as well. But if we think to use xid generated on subscriber then
> > we need to keep some mapping of original GID sent by publisher and GID
> > generated by us (origin+xid of subscription) because, at commit
> > prepared time, we won't know that xid.
>
> I agree that if we use (publisher's xid + subscriber origin id)
> instead of GID,  we can resolve this deadlock issue.
>

Yeah, the two things to keep in mind with this solution as well are
(a) still it is possible that conflict can be generated if the user
has prepared the transaction with that name of subscriber, the chances
of which are bleak and the user can always commit/rollback the
conflicting GID; (b) the subscription has two publications at
different nodes and then there is some chance that both send the same
xid, again the chances of this are bleak.

I think even though in the above kind of cases there is a chance of
conflict but it won't be a deadlock kind of situation. So, I guess it
is better to do this solution, what do you think?

>  I was also
> thinking that is it okay to change the prepared transaction name on
> the subscriber? I mean instead of GID if we use some other name then
> imagine a case where a user has prepared some transaction on the
> publisher and then tries to commit that on the subscriber using the
> prepared transaction name, then it will not work.  But maybe this is
> not really a practical use case.  I mean why anyone would want to
> prepare a transaction on the publisher and commit that prepared
> transaction directly on the subscriber.
>

It is not clear to me either if for such a purpose we need to use the
same GID as provided by the publisher. I don't know if there is any
such use case but if there is one, maybe later we can provide an
option with a subscription to use GID provided by the publisher when
two_phase is enabled?

-- 
With Regards,
Amit Kapila.

Re: pg_upgrade failing for 200+ million Large Objects

[Tom Lane]

Jan Wieck  writes:
> On 3/8/21 11:58 AM, Tom Lane wrote:
>> So it seems like the path of least resistance is
>> (a) make pg_upgrade use --single-transaction when calling pg_restore
>> (b) document (better) how to get around too-many-locks failures.

> That would first require to fix how pg_upgrade is creating the 
> databases. It uses "pg_restore --create", which is mutually exclusive 
> with --single-transaction because we cannot create a database inside of 
> a transaction.

Ugh.

> All that aside, the entire approach doesn't scale.

Yeah, agreed.  When we gave large objects individual ownership and ACL
info, it was argued that pg_dump could afford to treat each one as a
separate TOC entry because "you wouldn't have that many of them, if
they're large".  The limits of that approach were obvious even at the
time, and I think now we're starting to see people for whom it really
doesn't work.

I wonder if pg_dump could improve matters cheaply by aggregating the
large objects by owner and ACL contents.  That is, do

select distinct lomowner, lomacl from pg_largeobject_metadata;

and make just *one* BLOB TOC entry for each result.  Then dump out
all the matching blobs under that heading.

A possible objection is that it'd reduce the ability to restore blobs
selectively, so maybe we'd need to make it optional.

Of course, that just reduces the memory consumption on the client
side; it does nothing for the locks.  Can we get away with releasing the
lock immediately after doing an ALTER OWNER or GRANT/REVOKE on a blob?

regards, tom lane

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Sat, Mar 20, 2021 at 04:13:47PM +0100, Tomas Vondra wrote:
> +++ b/src/backend/access/brin/brin_tuple.c
> @@ -213,10 +213,20 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, 
> BrinMemTuple *tuple,
>   (atttype->typstorage == TYPSTORAGE_EXTENDED ||
>atttype->typstorage == TYPSTORAGE_MAIN))
>   {
> + Datum   cvalue;
> + charcompression = 
> GetDefaultToastCompression();
>   Form_pg_attribute att = 
> TupleDescAttr(brdesc->bd_tupdesc,
>   
>   keyno);
> - Datum   cvalue = 
> toast_compress_datum(value,
> - 
>   att->attcompression);
> +
> + /*
> +  * If the BRIN summary and indexed attribute 
> use the same data
> +  * type, we can the same compression method. 
> Otherwise we have

can *use ?

> +  * to use the default method.
> +  */
> + if (att->atttypid == atttype->type_id)
> + compression = att->attcompression;

It would be more obvious to me if this said here: 
| else: compression = GetDefaultToastCompression

-- 
Justin

Re: pg_upgrade failing for 200+ million Large Objects

[Andrew Dunstan]

On 3/20/21 12:39 AM, Jan Wieck wrote:
> On 3/8/21 11:58 AM, Tom Lane wrote:
>> The answer up to now has been "raise max_locks_per_transaction enough
>> so you don't see the failure".  Having now consumed a little more
>> caffeine, I remember that that works in pg_upgrade scenarios too,
>> since the user can fiddle with the target cluster's postgresql.conf
>> before starting pg_upgrade.
>>
>> So it seems like the path of least resistance is
>>
>> (a) make pg_upgrade use --single-transaction when calling pg_restore
>>
>> (b) document (better) how to get around too-many-locks failures.
>
> That would first require to fix how pg_upgrade is creating the
> databases. It uses "pg_restore --create", which is mutually exclusive
> with --single-transaction because we cannot create a database inside
> of a transaction. On the way pg_upgrade also mangles the
> pg_database.datdba (all databases are owned by postgres after an
> upgrade; will submit a separate patch for that as I consider that a
> bug by itself).
>
> All that aside, the entire approach doesn't scale.
>
> In a hacked up pg_upgrade that does "createdb" first before calling
> pg_upgrade with --single-transaction. I can upgrade 1M large objects with
>     max_locks_per_transaction = 5300
>     max_connectinons=100
> which contradicts the docs. Need to find out where that math went off
> the rails because that config should only have room for 530,000 locks,
> not 1M. The same test fails with max_locks_per_transaction = 5200.
>
> But this would mean that one has to modify the postgresql.conf to
> something like 530,000 max_locks_per_transaction at 100
> max_connections in order to actually run a successful upgrade of 100M
> large objects. This config requires 26GB of memory just for locks. Add
> to that the memory pg_restore needs to load the entire TOC before even
> restoring a single object.
>
> Not going to work. But tests are still ongoing ...



I thought Tom's suggestion upthread:


> Would it be sane to have the backend not bother to
> take any locks in binary-upgrade mode?


was interesting. Could we do that on the restore side? After all, what
are we locking against in binary upgrade mode?


cheers


andrew


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

Re: Logical Replication vs. 2PC

[Amit Kapila]

On Sat, Mar 20, 2021 at 2:57 PM Markus Wanner
 wrote:
>
> On 20.03.21 03:17, Amit Kapila wrote:
> > Are you saying that users might use the same GID which we have
> > constructed internally (say by combining origin and xid: originid_xid)
> > and then there will be conflict while replaying such transactions?
>
> No, I was pondering about a user doing (in short sequence):
>
> ..
> PREPARE TRANSACTION 'foobar';
> COMMIT PREPARED 'foobar';
>
> BEGIN;
> ...
> PREPARE TRANSACTION 'foobar';
> COMMIT PREPARED 'foobar';
>
> > Right and even for one subscription that can lead to blocking
> > transactions. But isn't it similar to what we get for a primary key
> > violation while replaying transactions?
>
> Sure, it's a conflict that prevents application.  A primary key conflict
> may be different in that it does not eventually resolve, though.
>
> > In that case, we suggest users
> > remove conflicting rows, so in such cases, we can recommend users to
> > commit/rollback such prepared xacts?
>
> Right, if you use gids, you could ask the user to always provide unique
> identifiers and not reuse them on any other node.  That's putting the
> burden of coming up with unique identifiers on the user, but that's a
> perfectly fine and reasonable thing to do.  (Lots of other systems out
> there requiring a unique request id or such, which would get confused if
> you issue requests with duplicate ids.)
>

Right, but I guess in our case using user-provided GID will conflict
if we use multiple subscriptions on the same node. So, it is better to
generate a unique identifier like we are discussing here, something
like (origin_id of subscription + xid of the publisher). Do you see
any problem with that?

-- 
With Regards,
Amit Kapila.

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

On 3/20/21 11:45 AM, Tomas Vondra wrote:
> 
> 
> On 3/20/21 11:18 AM, Dilip Kumar wrote:
>> On Sat, Mar 20, 2021 at 3:05 PM Tomas Vondra
>>  wrote:
>>>
>>> Hi,
>>>
>>> I think this bit in brin_tuple.c is wrong:
>>>
>>> ...
>>> Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
>>>   keyno);
>>> Datum   cvalue = toast_compress_datum(value,
>>>   att->attcompression);
>>>
>>> The problem is that this is looking at the index descriptor (i.e. what
>>> types are indexed) instead of the stored type. For BRIN those may be
>>> only loosely related, which is why the code does this a couple lines above:
>>>
>>> /* We must look at the stored type, not at the index descriptor. */
>>> TypeCacheEntry *atttype
>>> = brdesc->bd_info[keyno]->oi_typcache[datumno];
>>
>> Ok, I was not aware of this.
>>
> 
> Yeah, the BRIN internal structure is not obvious, and the fact that all
> the built-in BRIN variants triggers the issue makes it harder to spot.
> 
>>> For the built-in BRIN opclasses this happens to work, because e.g.
>>> minmax stores two values of the original type. But it may not work for
>>> other out-of-core opclasses, and it certainly doesn't work for the new
>>> BRIN opclasses (bloom and minmax-multi).
>>
>> Okay
>>
>>> Unfortunately, the only thing we have here is the type OID, so I guess
>>> the only option is using GetDefaultToastCompression(). Perhaps we might
>>> include that into BrinOpcInfo too, in the future.
>>
>> Right, I think for now we can use default compression for this case.
>>
> 
> Good. I wonder if we might have "per type" preferred compression in the
> future, which would address this. But for now just using the default
> compression seems fine.
> 

Actually, we can be a bit smarter - when the data types match, we can
use the compression method defined for the attribute. That works fine
for all built-in BRIN opclasses, and it seems quite reasonable - if the
user picked a particular compression method for a column, it's likely
because the data compress better with that method. So why not use that
for the BRIN summary, when possible (even though the BRIN indexes tend
to be tiny).

Attached is a patch doing this. Barring objection I'll push that soon,
so that I can push the BRIN index improvements (bloom etc.).

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>From 7f3b29dae25b08f28cd84dbbeb069d0c7759fafc Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sat, 20 Mar 2021 10:24:00 +0100
Subject: [PATCH] Use valid compression method in brin_form_tuple

When compressing the BRIN summary, we can't simply use the compression
method from the indexed attribute.  The summary may use a different data
type, e.g. fixed-length attribute may have varlena summary, leading to
compression failures.  For the built-in BRIN opclasses this happens to
work, because the summary uses the same data type as the attribute.

When the data types match, we can inherit use the compression method
specified for the attribute (it's copied into the index descriptor).
Otherwise we don't have much choice and have to use the default one.

Author: Tomas Vondra
Discussion: https://postgr.es/m/e0367f27-392c-321a-7411-a58e1a7e4817%40enterprisedb.com
---
 src/backend/access/brin/brin_tuple.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index 0ab5712c71..279a7e5970 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -213,10 +213,20 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple,
 (atttype->typstorage == TYPSTORAGE_EXTENDED ||
  atttype->typstorage == TYPSTORAGE_MAIN))
 			{
+Datum	cvalue;
+char	compression = GetDefaultToastCompression();
 Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
 	  keyno);
-Datum		cvalue = toast_compress_datum(value,
-		  att->attcompression);
+
+/*
+ * If the BRIN summary and indexed attribute use the same data
+ * type, we can the same compression method. Otherwise we have
+ * to use the default method.
+ */
+if (att->atttypid == atttype->type_id)
+	compression = att->attcompression;
+
+cvalue = toast_compress_datum(value, compression);
 
 if (DatumGetPointer(cvalue) != NULL)
 {
-- 
2.30.2

Re: Using COPY FREEZE in pgbench

[Fabien COELHO]

Hello Tatsuo-san,


I have looked in the code of PQprotocolVersion. The only case in which
it returns 0 is there's no connection. Yes, you are right. Once the
connection has been successfuly established, there's no chance it
fails. So I agree with you.


Attached v3 patch addresses this.


The "g" item in the section describing initialization steps
(i.e. option -I). I'd suggest just to replace "COPY" with "COPY
FREEZE" in the sentence.


Ok. The section is needed to be modified.


This is also addressed in the patch.


V3 works for me and looks ok. I changed it to ready in the CF app.

--
Fabien.

Re: simplifying foreign key/RI checks

[Amit Langote]

On Mon, Mar 8, 2021 at 11:41 PM Amit Langote  wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane  wrote:
> >  I guess I'm disturbed by the idea
> > that we'd totally replace the implementation technology for only one
> > variant of foreign key checks.  That means that there'll be a lot
> > of minor details that don't act the same depending on context.  One
> > point I was just reminded of by [1] is that the SPI approach enforces
> > permissions checks on the table access, which I do not see being done
> > anywhere in your patch.  Now, maybe it's fine not to have such checks,
> > on the grounds that the existence of the RI constraint is sufficient
> > permission (the creator had to have REFERENCES permission to make it).
> > But I'm not sure about that.  Should we add SELECT permissions checks
> > to this code path to make it less different?
> >
> > In the same vein, the existing code actually runs the query as the
> > table owner (cf. SetUserIdAndSecContext in ri_PerformCheck), another
> > nicety you haven't bothered with.  Maybe that is invisible for a
> > pure SELECT query but I'm not sure I would bet on it.  At the very
> > least you're betting that the index-related operators you invoke
> > aren't going to care, and that nobody is going to try to use this
> > difference to create a security exploit via a trojan-horse index.
>
> How about we do at the top of ri_ReferencedKeyExists() what
> ri_PerformCheck() always does before executing a query, which is this:
>
>/* Switch to proper UID to perform check as */
>GetUserIdAndSecContext(_userid, _sec_context);
>SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
>   save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
>   SECURITY_NOFORCE_RLS);
>
> And then also check the permissions of the switched user on the scan
> target relation's schema (ACL_USAGE) and the relation itself
> (ACL_SELECT).
>
> IOW, this:
>
> +   Oid save_userid;
> +   int save_sec_context;
> +   AclResult   aclresult;
> +
> +   /* Switch to proper UID to perform check as */
> +   GetUserIdAndSecContext(_userid, _sec_context);
> +   SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
> +  save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
> +  SECURITY_NOFORCE_RLS);
> +
> +   /* Check namespace permissions. */
> +   aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel),
> + GetUserId(), ACL_USAGE);
> +   if (aclresult != ACLCHECK_OK)
> +   aclcheck_error(aclresult, OBJECT_SCHEMA,
> +  get_namespace_name(RelationGetNamespace(pk_rel)));
> +   /* Check the user has SELECT permissions on the referenced relation. */
> +   aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(),
> + ACL_SELECT);
> +   if (aclresult != ACLCHECK_OK)
> +   aclcheck_error(aclresult, OBJECT_TABLE,
> +  RelationGetRelationName(pk_rel));
>
> /*
>  * Extract the unique key from the provided slot and choose the equality
> @@ -414,6 +436,9 @@ ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel,
> index_endscan(scan);
> ExecDropSingleTupleTableSlot(outslot);
>
> +   /* Restore UID and security context */
> +   SetUserIdAndSecContext(save_userid, save_sec_context);
> +
> /* Don't release lock until commit. */
> index_close(idxrel, NoLock);

I've included these changes in the updated patch.

> > Shall we mention RLS restrictions?  If we don't worry about that,
> > I think REFERENCES privilege becomes a full bypass of RLS, at
> > least for unique-key columns.
>
> Seeing what check_enable_rls() does when running under the security
> context set by ri_PerformCheck(), it indeed seems that RLS is bypassed
> when executing these RI queries.  The following comment in
> check_enable_rls() seems to say so:
>
>  * InNoForceRLSOperation indicates that we should not apply RLS even
>  * if the table has FORCE RLS set - IF the current user is the owner.
>  * This is specifically to ensure that referential integrity checks
>  * are able to still run correctly.

I've added a comment to note that the new way of "selecting" the
referenced tuple effectively bypasses RLS, as is the case when
selecting via SPI.

> > I wonder also what happens if the referenced table isn't a plain
> > heap with a plain btree index.  Maybe you're accessing it at the
> > right level of abstraction so things will just work with some
> > other access methods, but I'm not sure about that.
>
> I believe that I've made ri_ReferencedKeyExists() use the appropriate
> APIs to scan the index, lock the returned table tuple, etc., but do
> you think we might be better served by introducing a new set of APIs
> for this use case?

I concur that by using the interfaces defined in genam.h and
tableam.h, patch accounts for cases involving other access 

Re: Log message for GSS connection is missing once connection authorization is successful.

[Michael Paquier]

On Sat, Mar 20, 2021 at 05:37:47PM +0900, Michael Paquier wrote:
> It seems to me that this would make the tests faster, that the test
> would not need to wait for the logging collector and that the code
> could just use slurp_file($node->logfile) to get the data it wants to
> check for a given pattern without looking at current_logfiles.  I also
> think that not using truncate() on the logfile generated has the
> disadvantage to make the code fuzzy for its verification once we
> introduce patterns close to each other, as there could easily be an
> overlap.  That's one problem that SQL pattern checks had to deal with
> in the past.  Thoughts?

And, in terms of code, this really simplifies things.  Please see the
attached that I would like to apply.
--
Michael
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 079321bbfc..38e9ef7b1f 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -20,7 +20,7 @@ use Time::HiRes qw(usleep);
 
 if ($ENV{with_gssapi} eq 'yes')
 {
-	plan tests => 34;
+	plan tests => 26;
 }
 else
 {
@@ -170,10 +170,7 @@ $node->append_conf(
 	'postgresql.conf', qq{
 listen_addresses = '$hostaddr'
 krb_server_keyfile = '$keytab'
-logging_collector = on
 log_connections = on
-# these ensure stability of test results:
-log_rotation_age = 0
 lc_messages = 'C'
 });
 $node->start;
@@ -212,29 +209,15 @@ sub test_access
 	# Verify specified log message is logged in the log file.
 	if ($expect_log_msg ne '')
 	{
-		my $current_logfiles = slurp_file($node->data_dir . '/current_logfiles');
-		note "current_logfiles = $current_logfiles";
-		like($current_logfiles, qr|^stderr log/postgresql-.*log$|,
-			 'current_logfiles is sane');
-
-		my $lfname = $current_logfiles;
-		$lfname =~ s/^stderr //;
-		chomp $lfname;
-
-		# might need to retry if logging collector process is slow...
-		my $max_attempts = 180 * 10;
-		my $first_logfile;
-		for (my $attempts = 0; $attempts < $max_attempts; $attempts++)
-		{
-			$first_logfile = slurp_file($node->data_dir . '/' . $lfname);
-			last if $first_logfile =~ m/\Q$expect_log_msg\E/;
-			usleep(100_000);
-		}
+		my $first_logfile = slurp_file($node->logfile);
 
 		like($first_logfile, qr/\Q$expect_log_msg\E/,
 			 'found expected log file content');
 	}
 
+	# Clean up any existing contents in the node's log file so as
+	# future tests don't step on each other's generated contents.
+	truncate $node->logfile, 0;
 	return;
 }
 


signature.asc
Description: PGP signature

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Sat, Mar 20, 2021 at 1:14 PM Justin Pryzby  wrote:
>
> See attached.

I have looked into your patches

- 0001 to 0005 and 0007 look fine to me so maybe you can merge them
all and create a fixup patch.  Thanks for fixing this, these were some
silly mistakes I made in my patch.
- 0006 is fine but not sure what is the advantage over what we have today?
- And, 0008 and 0009, I think my
0001-Fixup-dump-toast-compression-method.patch[1] is doing this in a
much simpler way, please have a look and let me know if you think that
has any problems and we need to do the way you are doing here?

[1] 
https://www.postgresql.org/message-id/CAFiTN-v7EULPqVJ-6J%3DzH6n0%2BkO%3DYFtgpte%2BFTre%3DWrwcWBBTA%40mail.gmail.com

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: pspg pager is finished

[Pavel Stehule]

so 20. 3. 2021 v 7:51 odesílatel Vik Fearing 
napsal:

> On 3/20/21 4:34 AM, Pavel Stehule wrote:
> > Hi
> >
> > I finished work on pspg.
> >
> > https://github.com/okbob/pspg
> >
> > Now it has special features like rows or block selection by mouse, and
> > export related data to file or to clipboard in csv or tsv or insert
> > formats. Some basic features like sorting data per selected columns are
> > possible too.
> >
> > I hope this tool will serve well, and so work with Postgres (or other
> > supported databases) in the terminal will be more comfortable and more
> > efficient.
>
> If this means active development on it is finished, I would like to see
> this integrated into the tree, perhaps even directly into psql itself
> (unless the user chooses a different pager).  It is that useful.
>

yes, - almost all my ideas are implemented - and I have no plans to write
pspg as a light spreadsheet. It is just a pager.

Unfortunately, the source code of pspg has not required postgres quality. I
wrote it mostly alone without reviews and without initial experience with
this kind of application. Some implemented interactive features are very
complex (for terminal applications), and not too simply understandable. The
code is not too bad, if I compare it with source code of "more" or "less",
but again, there was not any check of other eyes. There are not any regress
tests, so I don't think so integration too core can be a good idea. Review
of about 35000 lines can be terrible work, but this project needs it  to
move forward . On second hand, it uses a lot of Postgres C patterns. And
any new development can be more concentrated on quality and less to
research.

Although pspg has not Postgres quality, it is a good tool that is used by a
lot of people. Can be nice to be propagated inside Postgres documentation,
or some Postgres demos.

pspg is now in my private repository (and although it uses BSD licence), I
will be proud if it can be moved to some community repository, and if the
community takes more control and all rights to this project.

Now, I would work more on other projects than pspg - and then pspg will be
in maintenance mode. I'll fix all reported errors.


> Thank you, Pavel, for this work.
>

Thank you :)

Pavel

-- 
> Vik Fearing
>

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

On 3/20/21 11:18 AM, Dilip Kumar wrote:
> On Sat, Mar 20, 2021 at 3:05 PM Tomas Vondra
>  wrote:
>>
>> Hi,
>>
>> I think this bit in brin_tuple.c is wrong:
>>
>> ...
>> Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
>>   keyno);
>> Datum   cvalue = toast_compress_datum(value,
>>   att->attcompression);
>>
>> The problem is that this is looking at the index descriptor (i.e. what
>> types are indexed) instead of the stored type. For BRIN those may be
>> only loosely related, which is why the code does this a couple lines above:
>>
>> /* We must look at the stored type, not at the index descriptor. */
>> TypeCacheEntry *atttype
>> = brdesc->bd_info[keyno]->oi_typcache[datumno];
> 
> Ok, I was not aware of this.
> 

Yeah, the BRIN internal structure is not obvious, and the fact that all
the built-in BRIN variants triggers the issue makes it harder to spot.

>> For the built-in BRIN opclasses this happens to work, because e.g.
>> minmax stores two values of the original type. But it may not work for
>> other out-of-core opclasses, and it certainly doesn't work for the new
>> BRIN opclasses (bloom and minmax-multi).
> 
> Okay
> 
>> Unfortunately, the only thing we have here is the type OID, so I guess
>> the only option is using GetDefaultToastCompression(). Perhaps we might
>> include that into BrinOpcInfo too, in the future.
> 
> Right, I think for now we can use default compression for this case.
> 

Good. I wonder if we might have "per type" preferred compression in the
future, which would address this. But for now just using the default
compression seems fine.


regards

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

Re: [PATCH] Hooks at XactCommand level

[Julien Rouhaud]

On Fri, Mar 19, 2021 at 11:02:29PM +0100, Gilles Darold wrote:
> Le 12/03/2021 à 06:55, Julien Rouhaud a écrit :
> >
> 
> I don't think we need to pass any information at least for the rollback
> at statement level extension. All information needed are accessible and
> actually at abort_current_transaction_hook we only toggle a boolean to
> fire the rollback.

That's what I thought but I wanted to be sure.

So, I have nothing more to say about the patch itself.  At that point, I guess
that we can't keep postponing that topic, and we should either:

- commit this patch, or Álvaro's one based on a new grammar keyword for BEGIN
  (maybe without the GUC if that's the only hard blocker), assuming that there
  aren't any technical issue with those

- reject this patch, and I guess set in stone that vanilla postgres will
  never allow that.

Given the situation I'm not sure if I should mark the patch as Ready for
Committer or not.  I'll leave it as-is for now as Álvaro is already in Cc.

> I have rebased the patch.

Thanks!

Re: Logical Replication vs. 2PC

[Dilip Kumar]

On Sat, Mar 20, 2021 at 7:50 AM Amit Kapila  wrote:
>
> On Fri, Mar 19, 2021 at 9:22 PM Markus Wanner
>  wrote:

> So, I think you are using xid of publisher and origin_id of
> subscription to achieve uniqueness because both will be accessible in
> prepare and commit prepared. Right? If so, I think that will work out
> here as well. But if we think to use xid generated on subscriber then
> we need to keep some mapping of original GID sent by publisher and GID
> generated by us (origin+xid of subscription) because, at commit
> prepared time, we won't know that xid.

I agree that if we use (publisher's xid + subscriber origin id)
instead of GID,  we can resolve this deadlock issue.  I was also
thinking that is it okay to change the prepared transaction name on
the subscriber? I mean instead of GID if we use some other name then
imagine a case where a user has prepared some transaction on the
publisher and then tries to commit that on the subscriber using the
prepared transaction name, then it will not work.  But maybe this is
not really a practical use case.  I mean why anyone would want to
prepare a transaction on the publisher and commit that prepared
transaction directly on the subscriber.  Thoughts?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Sat, Mar 20, 2021 at 3:05 PM Tomas Vondra
 wrote:
>
> Hi,
>
> I think this bit in brin_tuple.c is wrong:
>
> ...
> Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
>   keyno);
> Datum   cvalue = toast_compress_datum(value,
>   att->attcompression);
>
> The problem is that this is looking at the index descriptor (i.e. what
> types are indexed) instead of the stored type. For BRIN those may be
> only loosely related, which is why the code does this a couple lines above:
>
> /* We must look at the stored type, not at the index descriptor. */
> TypeCacheEntry *atttype
> = brdesc->bd_info[keyno]->oi_typcache[datumno];

Ok, I was not aware of this.

> For the built-in BRIN opclasses this happens to work, because e.g.
> minmax stores two values of the original type. But it may not work for
> other out-of-core opclasses, and it certainly doesn't work for the new
> BRIN opclasses (bloom and minmax-multi).

Okay

> Unfortunately, the only thing we have here is the type OID, so I guess
> the only option is using GetDefaultToastCompression(). Perhaps we might
> include that into BrinOpcInfo too, in the future.

Right, I think for now we can use default compression for this case.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Columns correlation and adaptive query optimization

[Konstantin Knizhnik]

On 19.03.2021 20:32, Zhihong Yu wrote:

Hi,
In AddMultiColumnStatisticsForQual(),

+   /* Loop until we considered all vars */
+   while (vars != NULL)
...
+       /* Contruct list of unique vars */
+       foreach (cell, vars)

What if some cell / node, gets into the else block:

+               else
+               {
+                   continue;

and being left in vars. Is there a chance for infinite loop ?
It seems there should be a bool variable indicating whether any cell 
gets to the following:


+           vars = foreach_delete_current(vars, cell);

If no cell gets removed in the current iteration, the outer while loop 
should exit.


Each iteration of outer loop (while (vars != NULL))
process variables belonging to one relation.
We take "else continue" branch only if variable belongs to some other 
relation.

At first iteration of foreach (cell, vars)
variable "cols" is NULL and we always take first branch of the if.
In other words, at each iteration of outer loop we always make some 
progress in processing "vars" list and remove some elements

from this list. So infinite loop can never happen.



Cheers

On Fri, Mar 19, 2021 at 9:58 AM Konstantin Knizhnik 
mailto:k.knizh...@postgrespro.ru>> wrote:




On 19.03.2021 12:17, Yugo NAGATA wrote:
> On Wed, 10 Mar 2021 03:00:25 +0100
> Tomas Vondra mailto:tomas.von...@enterprisedb.com>> wrote:
>
>> What is being proposed here - an extension suggesting which
statistics
>> to create (and possibly creating them automatically) is certainly
>> useful, but I'm not sure I'd call it "adaptive query
optimization". I
>> think "adaptive" means the extension directly modifies the
estimates
>> based on past executions. So I propose calling it maybe "statistics
>> advisor" or something like that.
> I am also agree with the idea to implement this feature as a new
> extension for statistics advisor.
>
>> BTW Why is "qual" in
>>
>>    static void
>>    AddMultiColumnStatisticsForQual(void* qual, ExplainState *es)
>>
>> declared as "void *"? Shouldn't that be "List *"?
> When I tested this extension using TPC-H queries, it raised
segmentation
> fault in this function. I think the cause would be around this
argument.
>
> Regards,
> Yugo Nagata
>
Attached please find new version of the patch with
AddMultiColumnStatisticsForQual parameter type fix and one more fix
related with handling synthetic attributes.
I can not reproduce the crash on TPC-H queries, so if the problem
persists, can you please send me stack trace and may be some other
information helping to understand the reason of SIGSEGV?

Thanks in advance,
Konstantin

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

Hi,

I think this bit in brin_tuple.c is wrong:

...
Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
  keyno);
Datum   cvalue = toast_compress_datum(value,
  att->attcompression);

The problem is that this is looking at the index descriptor (i.e. what
types are indexed) instead of the stored type. For BRIN those may be
only loosely related, which is why the code does this a couple lines above:

/* We must look at the stored type, not at the index descriptor. */
TypeCacheEntry *atttype
= brdesc->bd_info[keyno]->oi_typcache[datumno];

For the built-in BRIN opclasses this happens to work, because e.g.
minmax stores two values of the original type. But it may not work for
other out-of-core opclasses, and it certainly doesn't work for the new
BRIN opclasses (bloom and minmax-multi).

Unfortunately, the only thing we have here is the type OID, so I guess
the only option is using GetDefaultToastCompression(). Perhaps we might
include that into BrinOpcInfo too, in the future.


regards

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

Re: Logical Replication vs. 2PC

[Markus Wanner]

On 20.03.21 03:17, Amit Kapila wrote:

Are you saying that users might use the same GID which we have
constructed internally (say by combining origin and xid: originid_xid)
and then there will be conflict while replaying such transactions?


No, I was pondering about a user doing (in short sequence):

..
PREPARE TRANSACTION 'foobar';
COMMIT PREPARED 'foobar';

BEGIN;
...
PREPARE TRANSACTION 'foobar';
COMMIT PREPARED 'foobar';


Right and even for one subscription that can lead to blocking
transactions. But isn't it similar to what we get for a primary key
violation while replaying transactions?


Sure, it's a conflict that prevents application.  A primary key conflict 
may be different in that it does not eventually resolve, though.



In that case, we suggest users
remove conflicting rows, so in such cases, we can recommend users to
commit/rollback such prepared xacts?


Right, if you use gids, you could ask the user to always provide unique 
identifiers and not reuse them on any other node.  That's putting the 
burden of coming up with unique identifiers on the user, but that's a 
perfectly fine and reasonable thing to do.  (Lots of other systems out 
there requiring a unique request id or such, which would get confused if 
you issue requests with duplicate ids.)


Regards

Markus

Re: [HACKERS] Custom compression methods

[Andres Freund]

On 2021-03-19 15:44:34 -0400, Robert Haas wrote:
> I committed the core patch (0003) with a bit more editing.  Let's see
> what the buildfarm thinks.

Congrats Dilip, Robert, All. The slow toast compression has been a
significant issue for a long time.

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Hannu Krosing]

It would be really convenient if user-visible serialisations of the query
id had something that identifies the computation method.

maybe prefix 'N' for internal, 'S' for pg_stat_statements etc.

This would immediately show in logs at what point the id calculator was
changed

On Fri, Mar 19, 2021 at 5:54 PM Bruce Momjian  wrote:

> On Fri, Mar 19, 2021 at 10:27:51PM +0800, Julien Rouhaud wrote:
> > On Fri, Mar 19, 2021 at 09:29:06AM -0400, Bruce Momjian wrote:
> > > OK, that makes perfect sense.  I think the best solution is to document
> > > that compute_query_id just controls the built-in computation of the
> > > query id, and that extensions can also compute it if this is off, and
> > > pg_stat_activity and log_line_prefix will display built-in or extension
> > > computed query ids.
> >
> > So the last version of the patch should implement that behavior right?
> It's
> > just missing some explicit guidance that third-party extensions should
> only
> > calculate a queryid if compute_query_id is off
>
> Yes, I think we are now down to just how the extensions should be told
> to behave, and how we document this --- see the email I just sent.
>
> --
>   Bruce Momjian  https://momjian.us
>   EDB  https://enterprisedb.com
>
>   If only the physical world exists, free will is an illusion.
>
>
>
>

Re: shared memory stats: high level design decisions: consistency, dropping

[Hannu Krosing]

> But now we could instead schedule stats to be removed at commit
time. That's not trivial of course, as we'd need to handle cases where
the commit fails after the commit record, but before processing the
dropped stats.

We likely can not remove them at commit time, but only after the
oldest open snapshot moves parts that commit ?

Would an approach where we keep stats in a structure logically similar
to MVCC we use for normal tables be completely unfeasible ?

We would only need to keep one Version per backend in transaction .

---
Hannu




On Sat, Mar 20, 2021 at 12:51 AM Andres Freund  wrote:
>
> Hi,
>
> I am working on Kyotaro Horiguchi's shared memory stats patch [1] with
> the goal of getting it into a shape that I'd be happy to commit.  That
> thread is quite long and most are probably skipping over new messages in
> it.
>
> There are two high-level design decisions / questions that I think
> warrant a wider audience (I'll keep lower level discussion in the other
> thread).
>
> In case it is not obvious, the goal of the shared memory stats patch is
> to replace the existing statistics collector, to which new stats are
> reported via an UDP socket, and where clients read data from the stats
> collector by reading the entire database's stats from disk.
>
> The replacement is to put the statistics into a shared memory
> segment. Fixed-size stats (e.g. bgwriter, checkpointer, wal activity,
> etc) being stored directly in a struct in memory. Stats for objects
> where a variable number exists, e.g. tables, are addressed via a dshash.
> table that points to the stats that are in turn allocated using dsa.h.
>
>
> 1) What kind of consistency do we want from the pg_stats_* views?
>
> Right now the first access to stats in a transaction will trigger a read
> of both the global and per-database stats from disk. If the on-disk
> state is too old, we'll ask the stats collector to write out a new file
> a couple times.
>
> For the rest of the transaction that in-memory state is used unless
> pg_stat_clear_snapshot() is called. Which means that multiple reads from
> e.g. pg_stat_user_tables will show the same results as before [2].
>
> That makes stats accesses quite expensive if there are lots of
> objects.
>
> But it also means that separate stats accesses - which happen all the
> time - return something repeatable and kind of consistent.
>
> Now, the stats aren't really consistent in the sense that they are
> really accurate, UDP messages can be lost, or only some of the stats
> generated by a TX might not yet have been received, other transactions
> haven't yet sent them. Etc.
>
>
> With the shared memory patch the concept of copying all stats for the
> current database into local memory at the time of the first stats access
> doesn't make sense to me.  Horiguchi-san had actually implemented that,
> but I argued that that would be cargo-culting an efficiency hack
> required by the old storage model forward.
>
> The cost of doing this is substantial. On master, with a database that
> contains 1 million empty tables, any stats access takes ~0.4s and
> increases memory usage by 170MB.
>
>
> 1.1)
>
> I hope everybody agrees with not requiring that stats don't need to be
> the way they were at the time of first stat access in a transaction,
> even if that first access was to a different stat object than the
> currently accessed stat?
>
>
> 1.2)
>
> Do we expect repeated accesses to the same stat to stay the same through
> the transaction?  The easiest way to implement stats accesses is to
> simply fetch the stats from shared memory ([3]). That would obviously
> result in repeated accesses to the same stat potentially returning
> changing results over time.
>
> I think that's perfectly fine, desirable even, for pg_stat_*.
>
>
> 1.3)
>
> What kind of consistency do we expect between columns of views like
> pg_stat_all_tables?
>
> Several of the stats views aren't based on SRFs or composite-type
> returning functions, but instead fetch each stat separately:
>
> E.g. pg_stat_all_tables:
>  SELECT c.oid AS relid,
> n.nspname AS schemaname,
> c.relname,
> pg_stat_get_numscans(c.oid) AS seq_scan,
> pg_stat_get_tuples_returned(c.oid) AS seq_tup_read,
> sum(pg_stat_get_numscans(i.indexrelid))::bigint AS idx_scan,
> sum(pg_stat_get_tuples_fetched(i.indexrelid))::bigint + 
> pg_stat_get_tuples_fetched(c.oid) AS idx_tup_fetch,
> pg_stat_get_tuples_inserted(c.oid) AS n_tup_ins,
> ...
> pg_stat_get_autoanalyze_count(c.oid) AS autoanalyze_count
>FROM pg_class c
>  LEFT JOIN pg_index i ON c.oid = i.indrelid
> ...
>
> Which means that if we do not cache stats, additional stats updates
> could have been applied between two stats accessors. E.g the seq_scan
> from before some pgstat_report_stat() but the seq_tup_read from after.
>
> If we instead fetch all of a table's stats in one go, we would get
> consistency between the columns. But obviously that'd require changing
> all the stats views.

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

On Fri, Mar 19, 2021 at 04:38:03PM -0400, Robert Haas wrote:
> On Fri, Mar 19, 2021 at 4:20 PM Tom Lane  wrote:
> > Nope ... crake's displeased with your assumption that it's OK to
> > clutter dumps with COMPRESSION clauses.  As am I: that is going to
> > be utterly fatal for cross-version transportation of dumps.
> 
> Regarding your point, that does look like clutter. We don't annotate
> the dump with a storage clause unless it's non-default, so probably we
> should do the same thing here. I think I gave Dilip bad advice here...

On Fri, Mar 19, 2021 at 05:49:37PM -0400, Robert Haas wrote:
> Here's a patch for that. It's a little strange because you're going to
> skip dumping the toast compression based on the default value on the
> source system, but that might not be the default on the system where
> the dump is being restored, so you could fail to recreate the state
> you had. That is avoidable if you understand how things work, but some
> people might not. I don't have a better idea, though, so let me know
> what you think of this.

On Fri, Mar 19, 2021 at 07:22:42PM -0300, Alvaro Herrera wrote:
> Do you mean the column storage strategy, attstorage?  I don't think
> that's really related, because the difference there is not a GUC setting
> but a compiled-in default for the type.  In the case of compression, I'm
> not sure it makes sense to do it like that, but I can see the clutter
> argument: if we dump compression for all columns, it's going to be super
> noisy.
> 
> (At least, for binary upgrade surely you must make sure to apply the
> correct setting regardless of defaults on either system).
> 
> Maybe it makes sense to dump the compression clause if it is different
> from pglz, regardless of the default on the source server.  Then, if the
> target server has chosen lz4 as default, *all* columns are going to end
> up as lz4, and if it hasn't, then only the ones that were lz4 in the
> source server are going to.  That seems reasonable behavior.  Also, if
> some columns are lz4 in source, and target does not have lz4, then
> everything is going to work out to not-lz4 with just a bunch of errors
> in the output.

I think what's missing is dumping the GUC value itself, and then also dump any
columns that differ from the GUC's setting.  An early version of the GUC patch
actually had an "XXX" comment about pg_dump support, and I was waiting for a
review before polishing it.  This was modelled after default_tablespace and
default_table_access_method - I've mentioned that before that there's no
pg_restore --no-table-am, and I have an unpublished patch to add it.  That may
be how I missed this until now.

Then, this will output COMPRESSION on "a" (x)or "b" depending on the current
default:
| CREATE TABLE a(a text compression lz4, b text compression pglz);
When we restore it, we set the default before restoring columns.

I think it may be a good idea to document that dumps of columns with
non-default compression aren't portable to older server versions, or servers
--without-lz4.  This is a consequence of the CREATE command being a big text
blob, so pg_restore can't reasonably elide the COMPRESSION clause.

While looking at this, I realized that someone added the GUC to
postgresql.conf.sample, but not to doc/ - this was a separate patch until
yesterday.

I think since we're not doing catalog access for "pluggable" compression, this
should just be an enum GUC, with #ifdef LZ4.  Then we don't need a hook to
validate it.

ALTER and CREATE are silently accepting bogus compression names.

I can write patches for these later.

-- 
Justin

Re: Log message for GSS connection is missing once connection authorization is successful.

[Michael Paquier]

On Wed, Dec 02, 2020 at 02:44:31PM -0500, Stephen Frost wrote:
> And committed.

This has been committed as of dc11f31a, that changed the configuration
of the node in the kerberos test to use logging_collector.  Wouldn't
it be simpler to not use the logging collector here and use a logic
similar to what we do in PostgresNode::issues_sql_like() where we
truncate the log file before checking for a pattern?

It seems to me that this would make the tests faster, that the test
would not need to wait for the logging collector and that the code
could just use slurp_file($node->logfile) to get the data it wants to
check for a given pattern without looking at current_logfiles.  I also
think that not using truncate() on the logfile generated has the
disadvantage to make the code fuzzy for its verification once we
introduce patterns close to each other, as there could easily be an
overlap.  That's one problem that SQL pattern checks had to deal with
in the past.  Thoughts?
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Sat, Mar 20, 2021 at 1:22 PM Dilip Kumar  wrote:
>
> On Sat, Mar 20, 2021 at 8:11 AM Robert Haas  wrote:
> >
> > On Fri, Mar 19, 2021 at 8:25 PM Tom Lane  wrote:
> > > Extrapolating from the way we've dealt with similar issues
> > > in the past, I think the structure of pg_dump's output ought to be:
> > >
> > > 1. SET default_toast_compression = 'source system's value'
> > > in among the existing passel of SETs at the top.  Doesn't
> > > matter whether or not that is the compiled-in value.
> > >
> > > 2. No mention of compression in any CREATE TABLE command.
> > >
> > > 3. For any column having a compression option different from
> > > the default, emit ALTER TABLE SET ... to set that option after
> > > the CREATE TABLE.  (You did implement such a SET, I trust.)
> >
> > Actually, *I* didn't implement any of this. But ALTER TABLE sometab
> > ALTER somecol SET COMPRESSION somealgo works.
> >
> > This sounds like a reasonable approach.
>
> The attached patch implements that.

After sending this, just saw Justin also included patches for this.  I
think the ALTER ..SET COMPRESSION is more or less similar, I just
fetched it from the older version of the patch set.  But SET
default_toast_compression are slightly different.  I will look into
your version and provide my opinion on which one looks better and we
can commit that and feel free to share your thoughts.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Sat, Mar 20, 2021 at 8:11 AM Robert Haas  wrote:
>
> On Fri, Mar 19, 2021 at 8:25 PM Tom Lane  wrote:
> > Extrapolating from the way we've dealt with similar issues
> > in the past, I think the structure of pg_dump's output ought to be:
> >
> > 1. SET default_toast_compression = 'source system's value'
> > in among the existing passel of SETs at the top.  Doesn't
> > matter whether or not that is the compiled-in value.
> >
> > 2. No mention of compression in any CREATE TABLE command.
> >
> > 3. For any column having a compression option different from
> > the default, emit ALTER TABLE SET ... to set that option after
> > the CREATE TABLE.  (You did implement such a SET, I trust.)
>
> Actually, *I* didn't implement any of this. But ALTER TABLE sometab
> ALTER somecol SET COMPRESSION somealgo works.
>
> This sounds like a reasonable approach.

The attached patch implements that.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 40b53e24932b1f9203092a7f6972804af5a7a45b Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Sat, 20 Mar 2021 13:14:39 +0530
Subject: [PATCH] Fixup, dump toast compression method

---
 src/bin/pg_dump/pg_backup.h  |  1 +
 src/bin/pg_dump/pg_backup_archiver.c |  4 +++
 src/bin/pg_dump/pg_dump.c| 66 ++--
 src/bin/pg_dump/t/002_pg_dump.pl | 12 +++
 4 files changed, 52 insertions(+), 31 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 0296b9b..02019fc 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -210,6 +210,7 @@ typedef struct Archive
 	/* other important stuff */
 	char	   *searchpath;		/* search_path to set during restore */
 	char	   *use_role;		/* Issue SET ROLE to this */
+	char	   *default_toast_compression;
 
 	/* error handling */
 	bool		exit_on_error;	/* whether to exit on SQL errors... */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 1f82c64..a25f4d1 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3152,6 +3152,10 @@ _doSetFixedOutputState(ArchiveHandle *AH)
 	else
 		ahprintf(AH, "SET row_security = off;\n");
 
+	/* Select the dump-time default toast compression */
+	if (AH->public.default_toast_compression)
+		ahprintf(AH, "SET default_toast_compression = '%s';\n",
+ AH->public.default_toast_compression);
 	ahprintf(AH, "\n");
 }
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f8bec3f..7bbcaac 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1248,6 +1248,21 @@ setup_connection(Archive *AH, const char *dumpencoding,
 
 		AH->sync_snapshot_id = get_synchronized_snapshot(AH);
 	}
+
+	/*
+	 * Get default TOAST compression method, but not if the server's too
+	 * old to support the feature or if the user doesn't want to dump that
+	 * information anyway.
+	 */
+	if (AH->remoteVersion >= 14 && !dopt->no_toast_compression)
+	{
+		PGresult   *res;
+
+		res = ExecuteSqlQueryForSingleRow(AH,
+		  "SHOW default_toast_compression");
+		AH->default_toast_compression = pg_strdup(PQgetvalue(res, 0, 0));
+		PQclear(res);
+	}
 }
 
 /* Set up connection for a parallel worker process */
@@ -15905,31 +15920,6 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
 		  tbinfo->atttypnames[j]);
 	}
 
-	/*
-	 * Attribute compression
-	 */
-	if (!dopt->no_toast_compression &&
-		tbinfo->attcompression != NULL)
-	{
-		char	   *cmname;
-
-		switch (tbinfo->attcompression[j])
-		{
-			case 'p':
-cmname = "pglz";
-break;
-			case 'l':
-cmname = "lz4";
-break;
-			default:
-cmname = NULL;
-break;
-		}
-
-		if (cmname != NULL)
-			appendPQExpBuffer(q, " COMPRESSION %s", cmname);
-	}
-
 	if (print_default)
 	{
 		if (tbinfo->attgenerated[j] == ATTRIBUTE_GENERATED_STORED)
@@ -16348,6 +16338,32 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
   qualrelname,
   fmtId(tbinfo->attnames[j]),
   tbinfo->attfdwoptions[j]);
+
+			/*
+			 * Dump per-column compression options
+			 */
+			if (!dopt->no_toast_compression && tbinfo->attcompression != NULL)
+			{
+char	   *cmname;
+
+switch (tbinfo->attcompression[j])
+{
+	case 'p':
+		cmname = "pglz";
+		break;
+	case 'l':
+		cmname = "lz4";
+		break;
+	default:
+		cmname = NULL;
+		break;
+}
+
+if (cmname != NULL &&
+	strcmp(cmname, fout->default_toast_compression) != 0)
+	appendPQExpBuffer(q, "ALTER TABLE %s ALTER COLUMN %s\nSET COMPRESSION %s;\n",
+		qualrelname, fmtId(tbinfo->attnames[j]), cmname);
+			}
 		}
 
 		if (ftoptions)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index bc91bb1..737e464 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ 

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

See attached.

One issue is that the pg_dump tests are no longer exercising the COMPRESSION
clause.  I don't know how to improve on that, since lz4 may not be available.

..unless we changed attcompression='\0' to mean (for varlena) "the default
compression".  Rather than "resolving" to the default compression at the time
the table is created, columns without an explicit compression set would "defer"
to the GUC (of course, that only affects newly-inserted data).

Then, I think pg_dump would generate an COMPRESSION clause for columns with any
compression other than a null byte, and then the tests could "SET COMPRESSION
pglz" and check the output, since it's set to a specific compression, not just
inheriting the default.

I'm not sure if that'd be desirable, but I think that's similar to tablespaces,
where (if I recall) reltablespace=0 means "this database's default tblspc".

-- 
Justin
>From 58335318a9e72d0dbdf8c2009ba6d195a6cba862 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 19:12:53 -0500
Subject: [PATCH 1/9] Add docs for default_toast_compression..

bbe0a81db69bd10bd166907c3701492a29aca294
---
 doc/src/sgml/config.sgml | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ee4925d6d9..5cb851a5eb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8151,6 +8151,29 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
  
 
+ 
+  default_toast_compression (string)
+  
+   default_toast_compression configuration parameter
+  
+  
+  
+   
+This parameter specifies the default compression method to use
+when compressing data in TOAST tables.
+It applies only to variable-width data types.
+It may be overriden by compression clauses in the
+CREATE command, or changed after the relation is
+created by ALTER TABLE ... SET COMPRESSION.
+
+The supported compression methods are pglz and
+(if configured at the time PostgreSQL was
+built) lz4.
+The default is pglz.
+   
+  
+ 
+
  
   temp_tablespaces (string)
   
-- 
2.17.0

>From ae52d1db7fa5f7731bc5c1170dfb2ec5c39b111a Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 21:52:28 -0500
Subject: [PATCH 2/9] doc: pg_dump --no-toast-compression

---
 doc/src/sgml/ref/pg_dump.sgml | 12 
 1 file changed, 12 insertions(+)

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bcbb7a25fb..4a521186fb 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -931,6 +931,18 @@ PostgreSQL documentation
   
  
 
+ 
+  --no-toast-compression
+  
+   
+Do not output commands to set TOASTcompression
+methods.
+With this option, all objects will be created using whichever
+compression method is the default during restore.
+   
+  
+ 
+
  
   --no-unlogged-table-data
   
-- 
2.17.0

>From c47fafc376f847f4463b146f467c6046e44e5afa Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 20:23:40 -0500
Subject: [PATCH 3/9] Compression method is an char not an OID

---
 src/backend/commands/tablecmds.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9b2800bf5e..8e756e59d5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7847,6 +7847,7 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 		index_close(indrel, lockmode);
 	}
 }
+
 /*
  * ALTER TABLE ALTER COLUMN SET STORAGE
  *
@@ -15070,7 +15071,7 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	AttrNumber	attnum;
 	char	   *compression;
 	char		typstorage;
-	Oid			cmoid;
+	char		cmethod;
 	Datum		values[Natts_pg_attribute];
 	bool		nulls[Natts_pg_attribute];
 	bool		replace[Natts_pg_attribute];
@@ -15111,9 +15112,9 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	memset(replace, false, sizeof(replace));
 
 	/* get the attribute compression method. */
-	cmoid = GetAttributeCompression(atttableform, compression);
+	cmethod = GetAttributeCompression(atttableform, compression);
 
-	atttableform->attcompression = cmoid;
+	atttableform->attcompression = cmethod;
 	CatalogTupleUpdate(attrel, >t_self, tuple);
 
 	InvokeObjectPostAlterHook(RelationRelationId,
@@ -15123,7 +15124,7 @@ ATExecSetCompression(AlteredTableInfo *tab,
 	ReleaseSysCache(tuple);
 
 	/* apply changes to the index column as well */
-	SetIndexStorageProperties(rel, attrel, attnum, cmoid, '\0', lockmode);
+	SetIndexStorageProperties(rel, attrel, attnum, cmethod, '\0', lockmode);
 	table_close(attrel, RowExclusiveLock);
 
 	/* make changes visible */
-- 
2.17.0

>From f577d25165815993b0289c75d5395d23409863cc Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 19 Mar 2021 

replication cleanup code incorrect way to use of HTAB HASH_REMOVE ?

[Peter Smith]

Hi,

I found some dubious looking HTAB cleanup code for replication streams
(see file:worker.c, function:stream_cleanup_files).

viz.

--
static void
stream_cleanup_files(Oid subid, TransactionId xid)
{
charpath[MAXPGPATH];
StreamXidHash *ent;

/* Remove the xid entry from the stream xid hash */
ent = (StreamXidHash *) hash_search(xidhash,
(void *) ,
HASH_REMOVE,
NULL);
/* By this time we must have created the transaction entry */
Assert(ent != NULL);

/* Delete the change file and release the stream fileset memory */
changes_filename(path, subid, xid);
SharedFileSetDeleteAll(ent->stream_fileset);
pfree(ent->stream_fileset);
ent->stream_fileset = NULL;

/* Delete the subxact file and release the memory, if it exist */
if (ent->subxact_fileset)
{
subxact_filename(path, subid, xid);
SharedFileSetDeleteAll(ent->subxact_fileset);
pfree(ent->subxact_fileset);
ent->subxact_fileset = NULL;
}
}
--

Notice how the code calls hash_search(... HASH_REMOVE ...), but then
it deferences the same ent that was returned from that function.

IIUC that is a violation of the hash_search API, whose function
comment (dynahash.c) clearly says not to use the return value in such
a way:

--
 * Return value is a pointer to the element found/entered/removed if any,
 * or NULL if no match was found.  (NB: in the case of the REMOVE action,
 * the result is a dangling pointer that shouldn't be dereferenced!)
--

~~

PSA my patch to correct this by firstly doing a HASH_FIND, then only
HASH_REMOVE after we've finished using the ent.


Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-Fix-for-stream_cleanup_files-HASH_REMOVE.patch
Description: Binary data

Re: pspg pager is finished

[Vik Fearing]

On 3/20/21 4:34 AM, Pavel Stehule wrote:
> Hi
> 
> I finished work on pspg.
> 
> https://github.com/okbob/pspg
> 
> Now it has special features like rows or block selection by mouse, and
> export related data to file or to clipboard in csv or tsv or insert
> formats. Some basic features like sorting data per selected columns are
> possible too.
> 
> I hope this tool will serve well, and so work with Postgres (or other
> supported databases) in the terminal will be more comfortable and more
> efficient.

If this means active development on it is finished, I would like to see
this integrated into the tree, perhaps even directly into psql itself
(unless the user chooses a different pager).  It is that useful.

Thank you, Pavel, for this work.
-- 
Vik Fearing

Re: a verbose option for autovacuum

[Michael Paquier]

On Sat, Mar 20, 2021 at 01:06:51PM +0900, Masahiko Sawada wrote:
> It's not bad but it seems redundant a bit to me. We pass the idx in
> spite of passing also Irel[idx] and &(vacrelstats->indstats[idx]). I
> think your first idea that is done in v4 patch (saving index names at
> the beginning of heap_vacuum_rel() for autovacuum logging purpose
> only) and the idea of deferring to close indexes until the end of
> heap_vacuum_rel() so that we can refer index name at autovacuum
> logging are more simple.

Okay.

> We need to initialize *stats with NULL here.

Right.  I am wondering why I did not get any complain here.

> If shared_indstats is NULL (e.g., we do " stats =
> vacrelstats->indstats[indnum];"), vacrelstats->indstats[indnum] is not
> updated since we pass  I think we should pass
> &(vacrelstats->indstats[indnum]) instead in this case.

If we get rid completely of this idea around indnum, that I don't
disagree with so let's keep just indname, you mean to keep the second
argument IndexBulkDeleteResult of vacuum_one_index() and pass down
&(vacrelstats->indstats[indnum]) as argument.  No objections from me
to just do that.

> Previously, we update the element of the pointer array of index
> statistics to the pointer pointing to either the local memory or DSM.
> But with the above change, we do that only when the index statistics
> are in the local memory. In other words, vacrelstats->indstats[i] is
> never updated if the corresponding index supports parallel indexes. I
> think this is not relevant with the change that we'd like to do here
> (i.e., passing indnum down).

Yeah, that looks like just some over-engineering design on my side.
Would you like to update the patch with what you think is most
adapted?
--
Michael


signature.asc
Description: PGP signature

Re: pspg pager is finished

[Pavel Stehule]

so 20. 3. 2021 v 4:45 odesílatel Julien Rouhaud  napsal:

> On Sat, Mar 20, 2021 at 04:34:30AM +0100, Pavel Stehule wrote:
> > Hi
> >
> > I finished work on pspg.
> >
> > https://github.com/okbob/pspg
> >
> > Now it has special features like rows or block selection by mouse, and
> > export related data to file or to clipboard in csv or tsv or insert
> > formats. Some basic features like sorting data per selected columns are
> > possible too.
> >
> > I hope this tool will serve well, and so work with Postgres (or other
> > supported databases) in the terminal will be more comfortable and more
> > efficient.
>
> Thanks a lot for that tool Pavel.  It has been my favorite psql pager for
> years!
>

Thank you

Pavel

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Fri, Mar 19, 2021 at 12:53:18PM -0400, Bruce Momjian wrote:
> 
> Well, given we don't really want to support multiple query id types
> being generated or displayed, the "error out" above should fix it. 
> 
> Let's do this --- tell extensions to error out if the query id is
> already set, either by compute_query_id or another extension.  If an
> extension wants to generate its own query id and store is internal to
> the extension, that is fine, but the server-displayed query id should be
> generated once and never overwritten by an extension.

Agreed, this will ensure that you won't dynamically change the queryid source.

We should also document that changing it requires a restart and calling
pg_stat_statements_reset() afterwards.

v19 adds some changes, plus extra documentation for pg_stat_statements about
the requirement for a queryid to be calculated, and a note that all documented
details only apply for in-core source.  I'm not sure if this is still the best
place to document those details anymore though.
>From bcc76fdcff0ac867b087706a14141ceadcb371bf Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Wed, 14 Oct 2020 02:11:37 +0800
Subject: [PATCH v19 1/3] Move pg_stat_statements query jumbling to core.

A new compute_query_id GUC is also added, to control whether a query identifier
should be computed by the core.  It's thefore now possible to disable core
queryid computation and use pg_stat_statements with a different algorithm to
compute the query identifier by using third-party module.

To ensure that a single source of query identifier can be used and is well
defined, modules that calculate a query identifier should throw an error if
compute_query_id is enabled or if a query idenfitier was already calculated.

Author: Julien Rouhaud
Reviewed-by: Bruce Momjian
Discussion: https://postgr.es/m/CA+8PKvQnMfOE-c3YLRwxOsCYXQDyP8VXs6CDtMZp1V4=d4l...@mail.gmail.com
---
 .../pg_stat_statements/pg_stat_statements.c   | 805 +
 .../pg_stat_statements.conf   |   1 +
 doc/src/sgml/config.sgml  |  26 +
 doc/src/sgml/pgstatstatements.sgml|  20 +-
 src/backend/parser/analyze.c  |  14 +-
 src/backend/tcop/postgres.c   |   6 +-
 src/backend/utils/misc/Makefile   |   1 +
 src/backend/utils/misc/guc.c  |  10 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/backend/utils/misc/queryjumble.c  | 834 ++
 src/include/parser/analyze.h  |   4 +-
 src/include/utils/guc.h   |   1 +
 src/include/utils/queryjumble.h   |  58 ++
 13 files changed, 996 insertions(+), 785 deletions(-)
 create mode 100644 src/backend/utils/misc/queryjumble.c
 create mode 100644 src/include/utils/queryjumble.h

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..498f2aa376 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -8,24 +8,9 @@
  * a shared hashtable.  (We track only as many distinct queries as will fit
  * in the designated amount of shared memory.)
  *
- * As of Postgres 9.2, this module normalizes query entries.  Normalization
- * is a process whereby similar queries, typically differing only in their
- * constants (though the exact rules are somewhat more subtle than that) are
- * recognized as equivalent, and are tracked as a single entry.  This is
- * particularly useful for non-prepared queries.
- *
- * Normalization is implemented by fingerprinting queries, selectively
- * serializing those fields of each query tree's nodes that are judged to be
- * essential to the query.  This is referred to as a query jumble.  This is
- * distinct from a regular serialization in that various extraneous
- * information is ignored as irrelevant or not essential to the query, such
- * as the collations of Vars and, most notably, the values of constants.
- *
- * This jumble is acquired at the end of parse analysis of each query, and
- * a 64-bit hash of it is stored into the query's Query.queryId field.
- * The server then copies this value around, making it available in plan
- * tree(s) generated from the query.  The executor can then use this value
- * to blame query costs on the proper queryId.
+ * As of Postgres 9.2, this module normalizes query entries.  As of Postgres
+ * 14, the normalization is done by the core if compute_query_id is enabled,
+ * or optionally by third-party modules.
  *
  * To facilitate presenting entries to users, we create "representative" query
  * strings in which constants are replaced with parameter symbols ($n), to
@@ -114,8 +99,6 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
 #define USAGE_DEALLOC_PERCENT	5	/* free this % of entries at once */
 #define IS_STICKY(c)	((c.calls[PGSS_PLAN] + c.calls[PGSS_EXEC]) == 0)
 
-#define JUMBLE_SIZE1024	/*