Re: [PATCH] Completed unaccent dictionary with many missing characters

[Michael Paquier]

On Thu, May 05, 2022 at 09:44:15PM +0200, Przemysław Sztoch wrote:
> Tom, I disagree with you because many similar numerical conversions are
> already taking place, e.g. 1/2, 1/4...

This part sounds like a valid argument to me.  unaccent.rules does
already the conversion of some mathematical signs, and the additions
proposed in the patch don't look that weird to me.  I agree with Peter
and Przemysław that this is reasonable.
--
Michael


signature.asc
Description: PGP signature

Re: has_wal_read_bug

[Noah Misch]

On Tue, May 17, 2022 at 11:50:51AM +1200, Thomas Munro wrote:
> 027_stream_regress.pl has:
> 
> if (PostgreSQL::Test::Utils::has_wal_read_bug)
> {
> # We'd prefer to use Test::More->builder->todo_start, but the bug causes
> # this test file to die(), not merely to fail.
> plan skip_all => 'filesystem bug';
> }
> 
> Is the die() referenced there the one from the system_or_bail() call
> that commit a096813b got rid of?

No, it was the 'croak "timed out waiting for catchup"',
e.g. 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tadarida&dt=2022-01-25%2016%3A56%3A26

> Here's a failure in 031_recovery_conflict.pl that smells like
> concurrent pread() corruption:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tadarida&dt=2022-05-16%2015%3A45%3A54
> 
> 2022-05-16 18:10:33.375 CEST [52106:1] LOG:  started streaming WAL
> from primary at 0/300 on timeline 1
> 2022-05-16 18:10:33.621 CEST [52105:5] LOG:  incorrect resource
> manager data checksum in record at 0/338FDC8
> 2022-05-16 18:10:33.622 CEST [52106:2] FATAL:  terminating walreceiver
> process due to administrator command

Agreed.  Here, too:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tadarida&dt=2022-05-09%2015%3A46%3A03

> Presumably we also need the has_wal_read_bug kludge in all these new
> tests that use replication.

That is an option.  One alternative is to reconfigure those three animals to
remove --enable-tap-tests.  Another alternative is to make the build system
skip all files of all TAP suites on affected systems.  Handling this on a
file-by-file basis seemed reasonable to me when only two files had failed that
way.  Now, five files have failed.  We have wait_for_catchup calls in
fifty-one files, and I wouldn't have chosen the has_wal_read_bug approach if I
had expected fifty-one files to eventually call it.  I could tolerate it,
though.

amcheck is using a wrong macro to check compressed-ness

[Kyotaro Horiguchi]

Hello.

While I looked into a patch, I noticed that check_tuple_attribute does
not run the check for compessed data even if a compressed data is
given.

check_tuple_attribute()
..
struct varatt_external toast_pointer;
..
VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
..
if (VARATT_IS_COMPRESSED(&toast_pointer))
{

Since toast_pointer is a varatt_exteral it should be
VARATT_EXTERNAL_IS_COMPRESSED instead.  Since the just following
corss-check is just the reverse of what the macro does, it is useless.

What do you think about the attached?  The problem code is new in
PG15.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 00ae35ca30a6d3168dc4905ab5ba9db1339fe377 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 17 May 2022 16:19:27 +0900
Subject: [PATCH] Use correct macro to check compressed-ness in amcheck

check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead.  Remove the following cross check for data size since the
ycondition cannot be true.
---
 contrib/amcheck/verify_heapam.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
    toast_pointer.va_rawsize,
    VARLENA_SIZE_LIMIT));
 
-	if (VARATT_IS_COMPRESSED(&toast_pointer))
+	if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
 	{
 		ToastCompressionId cmid;
 		bool		valid = false;
 
-		/* Compression should never expand the attribute */
-		if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
-			report_corruption(ctx,
-			  psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
-	   toast_pointer.va_valueid,
-	   VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
-	   toast_pointer.va_rawsize));
-
 		/* Compressed attributes should have a valid compression method */
 		cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
 		switch (cmid)
-- 
2.27.0

Re: Fix a typo in walreceiver.c

[Michael Paquier]

On Tue, May 17, 2022 at 10:38:23AM +0530, Bharath Rupireddy wrote:
> Attaching a tiny patch to fix a typo - replace primary_slotname with
> correct parameter name primary_slot_name in walreceiver.c code
> comments.

Yep, indeed.  Will fix after beta1 is stamped.
--
Michael


signature.asc
Description: PGP signature

Re: amcheck is using a wrong macro to check compressed-ness

[Michael Paquier]

On Tue, May 17, 2022 at 04:27:19PM +0900, Kyotaro Horiguchi wrote:
> What do you think about the attached?  The problem code is new in
> PG15.

Adding Robert in CC, as this has been added with bd807be.  I have
added an open item for now.
--
Michael


signature.asc
Description: PGP signature

create_help.pl treats as replaceable

[Kyotaro Horiguchi]

I found it annoying that sql_help.c contains a literal parameter as a
translatable string.

The cause is that create_help.pl treats match as a
replaceable. The attached excludes literals from translatable strings.

By a quick look it seems to me that the "match" in "COPY.. HEADER
match" is the first and only instance of a literal parameter as of
PG15.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 81770afd9df6f073c50f875b2dd540273ecc15fa Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 17 May 2022 17:37:33 +0900
Subject: [PATCH] Do not treat  words as translatable

create_help.pl extracts  words as translatable but actually
they are untranslatable.  Exclude such words from translatable words
so that translators don't mistakenly translate the words.
---
 src/bin/psql/create_help.pl | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/bin/psql/create_help.pl b/src/bin/psql/create_help.pl
index 1a9836cbcc..9919aaf4e7 100644
--- a/src/bin/psql/create_help.pl
+++ b/src/bin/psql/create_help.pl
@@ -138,6 +138,11 @@ foreach my $file (sort readdir DIR)
 
 		while ($cmdsynopsis =~ m!<(\w+)[^>]*>(.+?)]*>!)
 		{
+			if ($1 eq "literal")
+			{
+$cmdsynopsis =~ s!<(\w+)[^>]*>(.+?)]*>!\2!;
+next;
+			}
 			my $match = $2;
 			$match =~ s/<[^>]+>//g;
 			$match =~ s/%%/%/g;
-- 
2.27.0

RE: bogus: logical replication rows/cols combinations

[houzj.f...@fujitsu.com]

On Tuesday, May 17, 2022 2:53 PM Amit Kapila  wrote:
> 
> Few minor comments:
> ==
> 1.
> +  
> +   Names of table columns included in the publication. This contains all
> +   the columns of table when user didn't specify column list for the
> +   table.
> +  
> 
> Can we slightly change it to: "Names of table columns included in the
> publication. This contains all the columns of the table when the user
> didn't specify the column list for the table."
> 
> 2. Below comments needs to be removed from tablesync.c as we don't
> combine column lists after this patch.
> 
>  * For initial synchronization, column lists can be ignored in following
> * cases:
> *
> * 1) one of the subscribed publications for the table hasn't specified
> * any column list
> *
> * 2) one of the subscribed publications has puballtables set to true
> *
> * 3) one of the subscribed publications is declared as ALL TABLES IN
> * SCHEMA that includes this relation
> 
> 3.
> Note that we don't support the case where column list is different for
> + * the same table when combining publications. But we still need to check
> + * all the given publication-table mappings and report an error if any
> + * publications have different column list.
> 
> Can we change this comment to: "Note that we don't support the case
> where the column list is different for the same table when combining
> publications. But one can later change the publication so we still
> need to check all the given publication-table mappings and report an
> error if any publications have a different column list."?
> 
> 4. Can we add a test for different column lists if it is not already there?

Thanks for the comments.

Attach the new version patch which addressed all the above comments and
comments from Shi yu[1] and Osumi-san[2].

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

Best regards,
Hou zj





v3-0001-Extend-pg_publication_tables-to-display-column-list-.patch
Description:  v3-0001-Extend-pg_publication_tables-to-display-column-list-.patch


v3-0002-Prohibit-combining-publications-with-different-colum.patch
Description:  v3-0002-Prohibit-combining-publications-with-different-colum.patch

Expand palloc/pg_malloc API

[Peter Eisentraut]

This adds additional variants of palloc, pg_malloc, etc. that 
encapsulate common usage patterns and provide more type safety.


Examples:

-   result = (IndexBuildResult *) palloc(sizeof(IndexBuildResult));
+   result = palloc_obj(IndexBuildResult);

-   collector->tuples = (IndexTuple *) palloc(sizeof(IndexTuple) *
  collector->lentuples);
+   collector->tuples = palloc_array(IndexTuple, collector->lentuples);

One common point is that the new interfaces all have a return type that 
automatically matches what they are allocating, so you don't need any 
casts nor have to manually make sure the size matches the expected 
result.  Besides the additional safety, the notation is also more 
compact, as you can see above.


Inspired by the talloc library.

The interesting changes are in fe_memutils.h and palloc.h.  The rest of 
the patch is just randomly sprinkled examples to test/validate the new 
additions.From 9ba5753228e0fe9c1eca779e92795a29b23146af Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 17 May 2022 13:28:58 +0200
Subject: [PATCH] Expand palloc/pg_malloc API

This adds additional variants of palloc, pg_malloc, etc. that
encapsulate common usage patterns and provide more type safety.
Inspired by talloc library.
---
 src/include/common/fe_memutils.h | 46 ++
 src/include/utils/palloc.h   | 33 ++
 src/backend/access/brin/brin.c   | 14 
 src/backend/access/gin/ginfast.c | 17 --
 src/backend/commands/indexcmds.c | 44 
 src/backend/executor/nodeHash.c  | 57 +---
 src/bin/pg_dump/common.c | 24 +-
 src/bin/pg_dump/pg_backup_tar.c  | 10 +++---
 src/bin/psql/startup.c   |  6 ++--
 src/common/config_info.c |  2 +-
 src/common/controldata_utils.c   |  2 +-
 contrib/dblink/dblink.c  | 12 +++
 12 files changed, 163 insertions(+), 104 deletions(-)

diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index 0a4c9126ea..993e0b07cd 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -29,6 +29,39 @@ extern void *pg_malloc_extended(size_t size, int flags);
 extern void *pg_realloc(void *pointer, size_t size);
 extern void pg_free(void *pointer);
 
+/*
+ * Variants with easier notation and more type safety
+ */
+
+/*
+ * Allocate space for one object of type "type"
+ */
+#define pg_malloc_obj(type) ((type *) pg_malloc(sizeof(type)))
+#define pg_malloc0_obj(type) ((type *) pg_malloc0(sizeof(type)))
+
+/*
+ * Allocate space for one object of what "ptr" points to
+ */
+#ifdef HAVE_TYPEOF
+#define pg_malloc_ptrtype(ptr) ((typeof(ptr)) pg_malloc(sizeof(*(ptr
+#define pg_malloc0_ptrtype(ptr) ((typeof(ptr)) pg_malloc0(sizeof(*(ptr
+#else
+#define pg_malloc_ptrtype(ptr) pg_malloc(sizeof(*(ptr)))
+#define pg_malloc0_ptrtype(ptr) pg_malloc0(sizeof(*(ptr)))
+#endif
+
+/*
+ * Allocate space for "count" objects of type "type"
+ */
+#define pg_malloc_array(type, count) ((type *) pg_malloc(sizeof(type) * 
(count)))
+#define pg_malloc0_array(type, count) ((type *) pg_malloc0(sizeof(type) * 
(count)))
+
+/*
+ * Change size of allocation pointed to by "pointer" to have space for "count"
+ * objects of type "type"
+ */
+#define pg_realloc_array(pointer, type, count) ((type *) pg_realloc(pointer, 
sizeof(type) * (count)))
+
 /* Equivalent functions, deliberately named the same as backend functions */
 extern char *pstrdup(const char *in);
 extern char *pnstrdup(const char *in, Size size);
@@ -38,6 +71,19 @@ extern void *palloc_extended(Size size, int flags);
 extern void *repalloc(void *pointer, Size size);
 extern void pfree(void *pointer);
 
+#define palloc_obj(type) ((type *) palloc(sizeof(type)))
+#define palloc0_obj(type) ((type *) palloc0(sizeof(type)))
+#ifdef HAVE_TYPEOF
+#define palloc_ptrtype(ptr) ((typeof(ptr)) palloc(sizeof(*(ptr
+#define palloc0_ptrtype(ptr) ((typeof(ptr)) palloc0(sizeof(*(ptr
+#else
+#define palloc_ptrtype(ptr) palloc(sizeof(*(ptr)))
+#define palloc0_ptrtype(ptr) palloc0(sizeof(*(ptr)))
+#endif
+#define palloc_array(type, count) ((type *) palloc(sizeof(type) * (count)))
+#define palloc0_array(type, count) ((type *) palloc0(sizeof(type) * (count)))
+#define repalloc_array(pointer, type, count) ((type *) repalloc(pointer, 
sizeof(type) * (count)))
+
 /* sprintf into a palloc'd buffer --- these are in psprintf.c */
 extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
 extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) 
pg_attribute_printf(3, 0);
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index 332575f518..cbc9c11ffa 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -80,6 +80,39 @@ extern void *palloc_extended(Size size, int flags);
 extern pg_nodiscard void *repalloc(void *pointer, Size size);
 extern void pfree(void *pointer

Re: Collecting statistics about contents of JSONB columns

[Mahendra Singh Thalor]

On Fri, 11 Mar 2022 at 04:29, Nikita Glukhov 
wrote:
>
>
> On 04.02.2022 05:47, Tomas Vondra wrote:
>
> On 1/25/22 17:56, Mahendra Singh Thalor wrote:
> >
>
> ...
>
> For the last few days, I was trying to understand these patches, and
based on Tomas's suggestion, I was doing some performance tests.
>
> With the attached .SQL file, I can see that analyze is taking more time
with these patches.
>
> I haven't found the root cause of this but I feel that this time is due
to a loop of all the paths.
> In my test data, there is a total of 951 different-2 paths. While doing
analysis, first we check all the sample rows(3) and we collect all the
different-2 paths (here 951), and after that for every single path, we loop
over all the sample rows again to collect stats for a particular path. I
feel that these loops might be taking time.
>
> Thanks, I've been doing some performance tests too, and you're right it
takes quite a bit of time.
>
>
> That is absolutely not surprising, I have warned about poor performance
> in cases with a large number of paths.
>
>
> I agree the slowness is largely due to extracting all paths and then
processing them one by one - which means we have to loop over the tuples
over and over. In this case there's about 850k distinct paths extracted, so
we do ~850k loops over 30k tuples. That's gotta take time.
>
> I don't know what exactly to do about this, but I already mentioned we
may need to pick a subset of paths to keep, similarly to how we pick items
for MCV. I mean, if we only saw a path once or twice, it's unlikely to be
interesting enough to build stats for it. I haven't tried, but I'd bet most
of the 850k paths might be ignored like this.
>
> The other thing we might do is making it the loops more efficient. For
example, we might track which documents contain each path (by a small
bitmap or something), so that in the loop we can skip rows that don't
contain the path we're currently processing. Or something like that.
>
> Apart from this performance issue, I haven't found any crashes or issues.
>
>
> Well, I haven't seen any crashes either, but as I mentioned for complex
documents (2 levels, many distinct keys) the ANALYZE starts consuming a lot
of memory and may get killed by OOM. For example if you generate documents
like this
>
>  ./json-generate.py 3 2 8 1000 6 1000
>
> and then run ANALYZE, that'll take ages and it very quickly gets into a
situation like this (generated from gdb by calling MemoryContextStats on
TopMemoryContext): and then run ANALYZE, that'll take ages and it very
quickly gets into a situation like this (generated from gdb by calling
MemoryContextStats on TopMemoryContext):
>
> -
> TopMemoryContext: 80776 total in 6 blocks; 13992 free (18 chunks); 66784
used
>   ...
>   TopPortalContext: 8192 total in 1 blocks; 7656 free (0 chunks); 536 used
> PortalContext: 1024 total in 1 blocks; 488 free (0 chunks); 536 used:

>   Analyze: 472726496 total in 150 blocks; 3725776 free (4 chunks);
469000720 used
> Analyze Column: 921177696 total in 120 blocks; 5123256 free (238
chunks); 916054440 used
>   Json Analyze Tmp Context: 8192 total in 1 blocks; 5720 free (1
chunks); 2472 used
> Json Analyze Pass Context: 8192 total in 1 blocks; 7928 free
(0 chunks); 264 used
>   JSON analyze path table: 1639706040 total in 25084 blocks;
1513640 free (33 chunks); 1638192400 used
>   Vacuum: 8192 total in 1 blocks; 7448 free (0 chunks); 744 used
> ...
> Grand total: 3035316184 bytes in 25542 blocks; 10971120 free (352
chunks); 3024345064 used
> -
>
>
> Yes, that's backend 3GB of memory, out of which 1.6GB is in "analyze path
table" context, 400MB in "analyze" and 900MB in "analyze column" contexts.
I mean, that seems a bit excessive. And it grows over time, so after a
while my laptop gives up and kills the backend.
>
> I'm not sure if it's a memory leak (which would be fixable), or it's due
to keeping stats for all the extracted paths. I mean, in this particular
case we have 850k paths - even if stats are just 1kB per path,  that's
850MB. This requires more investigation.
>
> Thank you for the tests and investigation.
>
> I have tried to reduce memory consumption and speed up row scanning:
>
> 1. "JSON analyze path table" context contained ~1KB JsonPathAnlStats
>structure per JSON path in the global hash table.  I have moved
>JsonPathAnlStats to the stack of compute_json_stats(), and hash
>table now consumes ~70 bytes per path.
>
> 2. I have fixed copying of resulting JSONB stats into context, which
>reduced the size of "Analyze Column" context.
>
> 3. I have optimized consumption of single-pass algorithm by storing
>only value lists in the non-temporary context.  That helped to
>execute "2 64 64" test case in 30 seconds.  Single-pass is a
>bit faster in non-TOASTe

Re: Expand palloc/pg_malloc API

[Bharath Rupireddy]

On Tue, May 17, 2022 at 5:11 PM Peter Eisentraut
 wrote:
>
> This adds additional variants of palloc, pg_malloc, etc. that
> encapsulate common usage patterns and provide more type safety.
>
> Examples:
>
> -   result = (IndexBuildResult *) palloc(sizeof(IndexBuildResult));
> +   result = palloc_obj(IndexBuildResult);
>
> -   collector->tuples = (IndexTuple *) palloc(sizeof(IndexTuple) *
>collector->lentuples);
> +   collector->tuples = palloc_array(IndexTuple, collector->lentuples);
>
> One common point is that the new interfaces all have a return type that
> automatically matches what they are allocating, so you don't need any
> casts nor have to manually make sure the size matches the expected
> result.  Besides the additional safety, the notation is also more
> compact, as you can see above.
>
> Inspired by the talloc library.
>
> The interesting changes are in fe_memutils.h and palloc.h.  The rest of
> the patch is just randomly sprinkled examples to test/validate the new
> additions.

It seems interesting. Are we always type-casting explicitly the output
of palloc/palloc0? Does this mean the compiler takes care of
type-casting the returned void * to the target type?

I see lots of instances where there's no explicit type-casting to the
target variable type -
retval = palloc(sizeof(GISTENTRY));
Interval   *p = palloc(sizeof(Interval));
macaddr*v = palloc0(sizeof(macaddr)); and so on.

Regards,
Bharath Rupireddy.

Re: Tracking notnull attributes inside Var

[Ashutosh Bapat]

On Sun, May 15, 2022 at 8:41 AM Andy Fan  wrote:

>
> The var in RelOptInfo->reltarget should have nullable = 0 but the var in
> RelOptInfo->baserestrictinfo should have nullable = 1;  The beauty of this
> are: a). It can distinguish the two situations perfectly b). Whenever we want
> to know the nullable attribute of a Var for an expression, it is super easy to
> know. In summary, we need to maintain the nullable attribute at 2 different
> places. one is the before the filters are executed(baserestrictinfo, joininfo,
> ec_list at least).  one is after the filters are executed 
> (RelOptInfo.reltarget
> only?)

Thanks for identifying this. What you have written makes sense and it
might open a few optimization opportunities. But let me put down some
other thoughts here. You might want to take those into consideration
when designing your solution.

Do we want to just track nullable and non-nullable. May be we want
expand this class to nullable (var may be null), non-nullable (Var is
definitely non-NULL), null (Var will be always NULL).

But the other way to look at this is along the lines of equivalence
classes. Equivalence classes record the expressions which are equal in
the final result of the query. The equivalence class members are not
equal at all the stages of query execution.  But because they are
equal in the final result, we can impose that restriction on the lower
levels as well. Can we think of nullable in that fashion? If a Var is
non-nullable in the final result, we can impose that restriction on
the intermediate stages since rows with NULL values for that Var will
be filtered out somewhere. Similarly we could argue for null Var. But
knowledge that a Var is nullable in the final result does not impose a
NULL, non-NULL restriction on the intermediate stages. If we follow
this thought process, we don't need to differentiate Var at different
stages in query.

>
> Come to JoinRel, we still need to maintain the 2 different cases as well.
>
> As for the joinrel.reltarget, currently it looks up the inputrel's reltarget 
> to
> get the Var, so it is easy to inherit from Var->nullable from inputrel, but
> we need to consider the new  changes introduced by current join,
> Like new NOT nullable attributes because of join clauses OR new nullable
> attributes because of outer join.  Everything looks good for now.

Yes, if we want to maintain nullness at different stages in the query.

>
> The hard part is RelOptInfo.joininfo & root->eq_classes. All of them uses
> the shared RestrictInfo, and it is unclear which Var->nullable should be used 
> in
> them. To not provide a wrong answer, I think we can assume nullable=-1 
> (unknown)
> and let the upper layer decides what to do (do we have known use cases to use
> the nullable attribute here?).

I think what applies to baserestrictinfo and reltarget also applies to
joininfo and join's reltarget. There will be three stages - join
clauses, join quals and reltarget.

In EQs the Vars in RestrictInfo will come from joininfo but EQ member
Vars will derive their nullable-ness from corresponding reltarget. I
can be wrong though.

>
> More considerations about this strategy:
> 1. We might use more memory for different var copies, the only known cases
>RelOptInfo->reltarget for now.

When a Var is copied the whole expression tree needs to be copied.
That might be more memory than just copies of Var nodes.

> 2. _equalVar() has more complex semantics: shall we consider nulls or not.

This is interesting. It might have impact on set_plan_references and
planner's ability to search and match expressions.

But if we take the approach I have suggested earlier, this question
will not arise.

-- 
Best Wishes,
Ashutosh Bapat

Provide read-only access to system catalog tables

[Chirag Karkera]

Hi Team,

Appreciate your time to look into this.

I have a requirement, where a user has to be provided DDL access on the
schema (which is provided to the user) and as there is some development
work in process the user has to be provided the read only access on system
catalog tables (information_schema and pg_catalog)

I have surfed a lot of materials online, but did not get any solution for
the same.

Request you to share some valuable input on this.

Thank You.

Regards,
Chirag Karkera

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

[Amit Kapila]

On Fri, May 13, 2022 at 3:11 PM wangw.f...@fujitsu.com
 wrote:
>
> Attach the patches.(Only changed the patch for HEAD.).
>

 # publications
-{ oid => '6119', descr => 'get OIDs of tables in a publication',
+{ oid => '6119', descr => 'get OIDs of tables in one or more publications',
   proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
-  provolatile => 's', prorettype => 'oid', proargtypes => 'text',
-  proallargtypes => '{text,oid}', proargmodes => '{i,o}',
+  provolatile => 's', prorettype => 'oid', proargtypes => 'any',
+  proallargtypes => '{any,oid}', proargmodes => '{i,o}',

Won't our use case (input one or more publication names) requires the
parameter type to be 'VARIADIC text[]' instead of 'any'? I might be
missing something here so please let me know your reason to change the
type to 'any' from 'text'?

-- 
With Regards,
Amit Kapila.

Re: Provide read-only access to system catalog tables

[David G. Johnston]

On Tuesday, May 17, 2022, Chirag Karkera  wrote:
>
>
> the user has to be provided the read only access on system catalog tables
> (information_schema and pg_catalog)
>

All roles have this, no action required.

David J.

Estimation of recovery times in postgres

[Bharath Rupireddy]

Hi,

We often hear customers/users asking questions like - How much time
does it take for postgres to recover if it crashes now? How much time
does it take for a PITR to finish if it's started now with a specific
recovery target?  When will the recovery of a postgres server end? It
will be nice if the postgres can "somehow" answer these questions. I
know this is easier said than done. At a bare minimum, the postgres
can scan the WAL from the last checkpoint till end of WAL to see how
many WAL records need to be replayed and count in "some" IO costs,
average redo/replay/apply times etc. and provide an estimate something
like "recovery, if started at this moment, will take approximately X
amount of time". To answer these questions, postgres needs to have
information about the average replay time of WAL records which depends
on the type of WAL record (replay of different WAL records take
different amount of time at different times; for instance, replay of a
WAL record with many FPIs or data blocks touched takes different time
based on the shared buffers hit and misses, disk IO etc.). The
postgres can capture and save average replay time of each WAL record
type over a period of time and use it for estimates which is of course
a costly thing for the postgres to do while it's actually recovering.
Or we can feed in some average disk IO, replay costs, postgres can
scan the WAL records and provide the estimates.

If postgres has a way to estimate recovery times, it can also trigger
checkpoints based on it to keep the RTO/recovery times under limits.

I know there are lots of unclear points for now but I would like to
start a discussion and hear more thoughts from the community. Please
feel free to provide your inputs.

Regards,
Bharath Rupireddy.

Re: Provide read-only access to system catalog tables

[Chirag Karkera]

Thanks David for your reply!

But when i created a role i am not able to view objects under
information_schema.*

I mean I am not able to view the data, I can see only the column names.

Thanks.

Regards,
Chirag Karkera

On Tue, May 17, 2022 at 6:40 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

> On Tuesday, May 17, 2022, Chirag Karkera  wrote:
>>
>>
>> the user has to be provided the read only access on system catalog tables
>> (information_schema and pg_catalog)
>>
>
> All roles have this, no action required.
>
> David J.
>
>

Re: Provide read-only access to system catalog tables

[David G. Johnston]

On Tue, May 17, 2022 at 6:21 AM Chirag Karkera 
wrote:

> Thanks David for your reply!
>
> But when i created a role i am not able to view objects under
> information_schema.*
>
> I mean I am not able to view the data, I can see only the column names.
>
>>
>>
Which goes to demonstrate you have permissions.  But information_schema
uses the permissions of the executing user to decide what to show - it is
pre-filtered (and doesn't address PostgreSQL-only features).  If you need
less restrictive behavior your best bet is to just use the system
catalogs.  Those give you everything.

David J.

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

[Ranier Vilela]

Em seg., 16 de mai. de 2022 às 20:26, David Rowley 
escreveu:

> On Sun, 15 May 2022 at 09:47, Ranier Vilela  wrote:
> > At function load_relcache_init_file, there is an unnecessary function
> call,
> > to initialize pgstat_info pointer to NULL.
> >
> > MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));
>
> What seems to have happened here is the field was changed to become a
> pointer in 77947c51c.  It's not incorrect to use MemSet() to zero out
> the pointer field.  What it does probably do is confuse the casual
> reader into thinking the field is a struct rather than a pointer to
> one.   It's probably worth making that consistent with the other
> fields so nobody gets confused.
>
> Can you add a CF entry for PG16 for this so we come back to it after we
> branch?
>
Of course.
I will add it.

regards,
Ranier Vilela

Re: PostgreSQL 15 Beta 1 release announcement draft

[Jonathan S. Katz]

On 5/15/22 10:58 PM, Amit Kapila wrote:

On Sun, May 15, 2022 at 12:22 AM Jonathan S. Katz  wrote:


Please provide feedback no later than 2022-05-19 0:00 AoE[1].




[`recovery_prefetch`](https://www.postgresql.org/docs/15/runtime-config-wal.html#GUC-RECOVERY-PREFETCH)
that can help speed up all recovery operations by prefetching data blocks.


Is it okay to say that this feature speeds up *all* recovery
operations? See the discussion between Simon and Tomas [1] related to
this.


I'll all to hedge.

Thanks,

Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

Re: PostgreSQL 15 Beta 1 release announcement draft

[Jonathan S. Katz]

Thanks everyone for the feedback. As per usual, I did a `MERGE` based on 
the suggestions.


I provided credits in press.git. Here is v2 of the draft.

Please provide any feedback no later than 2022-05-19 0:00 AoE.

Thanks,

Jonathan
The PostgreSQL Global Development Group announces that the first beta release of
PostgreSQL 15 is now [available for 
download](https://www.postgresql.org/download/).
This release contains previews of all features that will be available when
PostgreSQL 15 is made generally available, though some details of the release
can change during the beta period.

You can find information about all of the features and changes found in
PostgreSQL 15 in the [release 
notes](https://www.postgresql.org/docs/15/release-15.html):

  
[https://www.postgresql.org/docs/15/release-15.html](https://www.postgresql.org/docs/15/release-15.html)

In the spirit of the open source PostgreSQL community, we strongly encourage you
to test the new features of PostgreSQL 15 on your systems to help us eliminate
bugs or other issues that may exist. While we do not advise you to run
PostgreSQL 15 Beta 1 in production environments, we encourage you to find ways
to run your typical application workloads against this beta release.

Your testing and feedback will help the community ensure that the PostgreSQL 15
release upholds our standards of delivering a stable, reliable release of the
world's most advanced open source relational database. Please read more about
our [beta testing process](https://www.postgresql.org/developer/beta/) and how
you can contribute:

  
[https://www.postgresql.org/developer/beta/](https://www.postgresql.org/developer/beta/)

PostgreSQL 15 Feature Highlights


### Developer Experience

PostgreSQL 15 adds new features for simplifying and enhancing the developer
experience.

This release introduces 
[`MERGE`](https://www.postgresql.org/docs/15/sql-merge.html),
a SQL standard command for conditionally performing write operations (`INSERT`,
`UPDATE`, or `DELETE`) on data. Prior to this release, the same behavior could
be accomplished either using stored procedures or, on a limited-basis, with
[`INSERT ... ON CONFLICT`](https://www.postgresql.org/docs/15/sql-insert.html).
With PostgreSQL 15, developers can write simple, expressive queries to choose
the appropriate data modification action to take.

PostgreSQL added support for JSON in 2012 as part of the [9.2 
release](https://www.postgresql.org/about/news/postgresql-92-released-1415/). 
The SQL/JSON standard, published five years
later, specified a variety of interfaces for accessing and manipulating JSON
data stored in relational databases. PostgreSQL 15 builds on its existing
support for the SQL/JSON path language by including more standard
[SQL/JSON 
functions](https://www.postgresql.org/docs/15/functions-json.html#FUNCTIONS-SQLJSON).
These include [SQL/JSON 
constructors](https://www.postgresql.org/docs/15/functions-json.html#FUNCTIONS-SQLJSON-PRODUCING),
[query / introspection functions](FUNCTIONS-SQLJSON-QUERYING),
and the ability to [convert JSON data into a 
table](https://www.postgresql.org/docs/15/functions-json.html#FUNCTIONS-JSONTABLE).

PostgreSQL 15 adds [more regular expression 
functions](https://www.postgresql.org/docs/15/functions-matching.html#FUNCTIONS-POSIX-REGEXP),
including `regexp_count` , `regexp_instr`, `regexp_like`, and `regexp_substr`.
the [`range_agg`](https://www.postgresql.org/docs/15/functions-aggregate.html)
function, introduced in PostgreSQL 15 for aggregating
[`range` data types](https://www.postgresql.org/docs/15/rangetypes.html) into
`multirange` types, now supports aggregating `multirange` types too.

### Performance

PostgreSQL 15 continues to build on its performance gains over the past several
releases. This release includes a significant speedup for sorting data when
sorting over larger data sets. In particular, these are data sets that exceed
the `work_mem` parameter. Early benchmarks show that these sorts may see on
average an 2x speedup for these workloads on PostgreSQL 15.

The performance gains of PostgreSQL 15 extend to a variety of query types.
This includes the introduction of parallelization for
[`SELECT 
DISTINCT`](https://www.postgresql.org/docs/15/queries-select-lists.html#QUERIES-DISTINCT)
statements and improvements in performance to
[window functions](https://www.postgresql.org/docs/15/functions-window.html)
that use `row_number()`, `rank()`, and `count()`. Applications that use the
[PostgreSQL foreign data 
wrapper](https://www.postgresql.org/docs/15/postgres-fdw.html)
[`postgres_fdw`](https://www.postgresql.org/docs/15/postgres-fdw.html) to manage
data on remote PostgreSQL servers can now enable
[transactions to be committed in 
parallel](https://www.postgresql.org/docs/15/postgres-fdw.html#id-1.11.7.47.11.7).
 There are also several performance enhancements for queries
involving tables with partitions.

PostgreSQL system and 
[TOAST](https://www.postg

Re: Provide read-only access to system catalog tables

[Chirag Karkera]

Thank you for the clarification.

Will use the system catalogs tables.

Thank You.

Regards,
Chirag Karkera

On Tue, May 17, 2022 at 6:59 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

> On Tue, May 17, 2022 at 6:21 AM Chirag Karkera 
> wrote:
>
>> Thanks David for your reply!
>>
>> But when i created a role i am not able to view objects under
>> information_schema.*
>>
>> I mean I am not able to view the data, I can see only the column names.
>>
>>>
>>>
> Which goes to demonstrate you have permissions.  But information_schema
> uses the permissions of the executing user to decide what to show - it is
> pre-filtered (and doesn't address PostgreSQL-only features).  If you need
> less restrictive behavior your best bet is to just use the system
> catalogs.  Those give you everything.
>
> David J.
>

Re: Removing unneeded self joins

[Ronan Dunklau]

Le vendredi 13 mai 2022, 07:07:47 CEST Andrey Lepikhov a écrit :
> New version of the feature.
> Here a minor bug with RowMarks is fixed. A degenerated case is fixed,
> when uniqueness of an inner deduced not from join quals, but from a
> baserestrictinfo clauses 'x=const', where x - unique field.
> Code, dedicated to solve second issue is controversial, so i attached
> delta.txt for quick observation.
> Maybe we should return to previous version of code, when we didn't split
> restriction list into join quals and base quals?

Hello,

I tried to find problematic cases, which would make the planning time grow 
unacceptably, and couldn't devise it.

The worst case scenario I could think of was as follows:

 - a query with many different self joins
 - an abundance of unique indexes on combinations of this table columns to 
consider
 - additional predicates on the where clause on columns.

The base table I used for this was a table with 40 integers. 39 unique indexes 
were defined on every combination of (c1, cX) with cX being columns c2 to c40.

I turned geqo off, set from_collapse_limit and join_collapse_limit to 
unreasonably high values (30), and tried to run queries of the form:

SELECT * FROM test_table t1
JOIN test_table tX ON t1.c1 = tX.c1 AND t1.c[X+2] = tX.cX
...
JOIN test_table tX ON t1.c1 = tX.c1 AND t1.c[X+2] = tX.cX.

So no self join can be eliminated in that case.
The performance was very similar with or without the GUC enabled. I tested the 
same thing without the patch, since the test for uniqueness has been slightly 
altered and incurs a new allocation, but it doesn't seem to change. 

One interesting side effect of this patch, is that removing any unneeded self 
join cuts down the planification time very significantly, as we lower the 
number 
of combinations to consider. 

As for the code: 

 - Comments on relation_has_unique_index_ext and relation_has_unique_index_for 
should be rewritten, as relation_has_unique_index_for is now just a special 
case of relation_has_unique_index_ext. By the way, the comment would probably 
be better read as: "but if extra_clauses isn't NULL".
 - The whole thing about "extra_clauses", ie, baserestrictinfos which were 
used to determine uniqueness, is not very clear. Most functions where the new 
argument has been added have not seen an update in their comments, and the 
name itself doesn't really convey the intented meaning: perhaps 
required_non_join_clauses ?

The way this works should be explained a bit more thoroughly, for example in 
remove_self_joins_one_group the purpose of uclauses should be explained. The 
fact that degenerate_case returns true when we don't have any additional base 
restrict info is also confusing, as well as the degenerate_case name.

I'll update if I think of more interesting things to add. 

-- 
Ronan Dunklau

Re: PostgreSQL 15 Beta 1 release announcement draft

[Justin Pryzby]

On Tue, May 17, 2022 at 08:58:14AM -0500, Jonathan S. Katz wrote:
> PostgreSQL 15 adds [more regular expression 
> functions](https://www.postgresql.org/docs/15/functions-matching.html#FUNCTIONS-POSIX-REGEXP),
> including `regexp_count` , `regexp_instr`, `regexp_like`, and `regexp_substr`.
> the [`range_agg`](https://www.postgresql.org/docs/15/functions-aggregate.html)
> function, introduced in PostgreSQL 15 for aggregating

Capital The

> the `work_mem` parameter. Early benchmarks show that these sorts may see on
> average an 2x speedup for these workloads on PostgreSQL 15.

Maybe remove "for these workloads".

> Write-ahead log (WAL) files can now be compressed using both LZ4 and Zstandard

Now that I re-read it, I suppose this should say "either .. or" ...

Re: create_help.pl treats as replaceable

[Tom Lane]

Kyotaro Horiguchi  writes:
> I found it annoying that sql_help.c contains a literal parameter as a
> translatable string.

> The cause is that create_help.pl treats match as a
> replaceable. The attached excludes literals from translatable strings.

> By a quick look it seems to me that the "match" in "COPY.. HEADER
> match" is the first and only instance of a literal parameter as of
> PG15.

Isn't that a documentation bug rather than a problem with create_help?
I see what you're talking about:

HEADER [ boolean | 
match ]

but that just seems flat-out wrong.  If "match" is a keyword it should
be rendered like other keywords.  I'm not very interested in splitting
hairs about whether the grammar thinks it is a keyword --- it looks like
one to a user.  So I think

HEADER [ boolean | MATCH ]

would be a better solution.

regards, tom lane

Re: Collecting statistics about contents of JSONB columns

[Tomas Vondra]

On 5/17/22 13:44, Mahendra Singh Thalor wrote:
> ...
>
> Hi Nikita,
> I and Tomas discussed the design for disabling all-paths
> collection(collect stats for only some paths). Below are some
> thoughts/doubts/questions.
> 
> *Point 1)* Please can you elaborate more that how are you going to
> implement this(collect stats for only some paths).

I think Nikita mentioned he plans to only build stats only for most
common paths, which seems generally straightforward:

1) first pass over the documents, collect distinct paths and track how
many times we saw each one

2) in the second pass extract stats only for the most common paths (e.g.
the top 100 most common ones, or whatever the statistics target says)

I guess we might store at least the frequencing for uncommon paths,
which seems somewhat useful for selectivity estimation.


I wonder if we might further optimize this for less common paths. AFAICS
one of the reasons why we process the paths one by one (in the second
pass) is to limit memory consumption. By processing a single path, we
only need to accumulate values for that path.

But if we know the path is uncommon, we know there'll be few values. For
example the path may be only in 100 documents, not the whole sample. So
maybe we might process multiple paths at once (which would mean we don't
need to detoast the JSON documents that often, etc.).

OTOH that may be pointless, because if the paths are uncommon, chances
are the subsets of documents will be different, in which case it's
probably cheaper to just process the paths one by one.


> *Point 2) *As JSON stats are taking time so should we add an on/off
> switch to collect JSON stats?

IMHO we should think about doing that. I think it's not really possible
to eliminate (significant) regressions for all corner cases, and in many
cases people don't even need this statistics (e.g. when just storing and
retrieving JSON docs, without querying contents of the docs).

I don't know how exactly to enable/disable this - it very much depends
on how we store the stats. If we store that in pg_statistic, then ALTER
TABLE ... ALTER COLUMN seems like the right way to enable/disable these
path stats. We might also have a new "json" stats and do this through
CREATE STATISTICS. Or something else, not sure.


> *Point 3)* We thought of one more design: we can give an explicit path
> to collect stats for a particular path only or we can pass a subset of
> the JSON values but this may require a lot of code changes like syntax
> and all so we are thinking that it will be good if we can collect stats
> only for some common paths(by limit or any other way)
> 

I'm not sure I understand what this is saying, particularly the part
about subset of JSON values. Can you elaborate?

I can imagine specifying a list of interesting paths, and we'd only
collect stats for the matching subset of the JSON documents. So if you
have huge JSON documents with complex schema, but you only query a very
limited subset of paths, we could restrict ANALYZE to this subset.

In fact, that's what the 'selective analyze' commit [1] in Nikita's
original patch set does in principle. We'd probably need to improve this
in some ways (e.g. to allow defining the column filter not just in
ANALYZE itself). I left it out of this patch to keep the patch as simple
as possible.

But why/how exactly would we limit the "JSON values"? Can you give some
example demonstrating that in practice?

regards



[1]
https://github.com/postgrespro/postgres/commit/7ab7397450df153e5a8563c978728cb731a0df33

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

Re: Add WAL recovery messages with log_wal_traffic GUC (was: add recovery, backup, archive, streaming etc. activity messages to server logs along with ps display)

[Robert Haas]

On Tue, May 17, 2022 at 1:32 AM Bharath Rupireddy
 wrote:
> In a production environment (of course with a better management of
> server logs) one can set log_wal_traffic to "high" and emit the
> required info to answer some of the customer questions like - "How far
> the server is in recovery? How much time recovery of each WAL file
> approximately took? How much time will it take to recover all the WAL
> files? What's the rate of recovery - time per WAL file? etc."
>
> Whereas ereport_startup_progress facility will help to emit log
> messages only if "some" operation takes longer than set
> log_startup_progress_interval time which may not serve the above
> purpose.

I think you are misunderstanding what an "operation" is here. If you
take the time to test out that facility, I think you will find that it
is quite capable of answering all of those questions.

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

Re: Remove support for Visual Studio 2013

[Juan José Santamaría Flecha]

On Mon, May 16, 2022 at 8:58 AM Michael Paquier  wrote:

>
> The patch attached cleans up the following things proper to VS 2013:
> - Locale handling.
> - MIN_WINNT assignment.
> - Some strtof() business, as of win32_port.h.
> - Removal of _set_FMA3_enable() in main.c related to floating-point
> operations.
> - MSVC scripts, but that's less interesting considering the work done
> with meson.
>
> When building on MinGW with NLS enabled I get some errors:

c:/cirrus/src/backend/utils/adt/pg_locale.c: In function
'search_locale_enum':
c:/cirrus/src/backend/utils/adt/pg_locale.c:989:13: warning: implicit
declaration of function 'GetLocaleInfoEx'; did you mean 'GetLocaleInfoA'?
[-Wimplicit-function-declaration]
  989 | if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHLANGUAGENAME,
  | ^~~
  | GetLocaleInfoA

This is because current MinGW defaults to Windows 2003 [1], maybe we should
fix Windows' minimal version to Vista (0x0600) unconditionally also. I have
seen a couple of compilation warnings while testing that setting on MinGW,
please find attached a patch for so.

[1]
https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sdkddkver.h

Regards,

Juan José Santamaría Flecha


0001-MinGW-Windows-Vista-Warnings.patch
Description: Binary data

Re: Tracking notnull attributes inside Var

[Tom Lane]

Andy Fan  writes:
> notnulls discussion is forked from UniqueKey stuff, you can see the
> attachment
> for the UnqiueKey introduction. Tom raised his opinion to track the
> nullability
> inside Var[1][2][3], this thread would start from there based on my
> understanding.

I'm pretty certain that I never suggested this:

> struct Var
> {
> ...;
>   int nullable;  // -1 unknown,  0 - not nullable.  1 - nullable
> };

You're free to pursue it if you like, but I think it will be a dead end.
The fundamental problem as you note is that equalVar() cannot do anything
sane with a field defined that way.  Also, we'd have to generate Vars
initially with nullable = unknown (else, for example, ALTER SET/DROP NOT
NULL breaks stored views referring to the column).  It'd be on the planner
to run through the tree and replace that with "nullable" or "not
nullable".  It's hard to see how that's more advantageous than just
keeping the info in the associated RelOptInfo.

Also, I think you're confusing two related but distinct issues.  For
certain optimization issues, we'd like to keep track of whether a column
stored in a table is known NOT NULL.  However, that's not the same thing
as the question that I've muttered about, which is how to treat a Var
that's been possibly forced to null due to null-extension of an outer
join.  That is a different value from the Var as read from the table,
but we currently represent it the same within the planner, which causes
various sorts of undesirable complication.  We cannot fix that by setting
Var.nullable = true in above-the-join instances, because it might also
be true in below-the-join instances.  "Known not null in the table" is
not the inverse of "potentially nulled by an outer join".  Moreover, we
probably need to know *which* join is the one potentially nulling the Var,
so a bool is not likely enough anyway.

The schemes I've been toying with tend to look more like putting a
PlaceHolderVar-ish wrapper around the Var or expression that represents
the below-the-join value.  The wrapper node could carry any join ID
info that we find necessary.  The thing that I'm kind of stalled on is
how to define this struct so that it's not a big headache for join
strength reduction (which could remove the need for a wrapper altogether)
or outer-join reordering (which makes it a bit harder to define which
join we think is the one nulling the value).

regards, tom lane

Re: Expand palloc/pg_malloc API

[Tom Lane]

Bharath Rupireddy  writes:
> On Tue, May 17, 2022 at 5:11 PM Peter Eisentraut
>  wrote:
>> This adds additional variants of palloc, pg_malloc, etc. that
>> encapsulate common usage patterns and provide more type safety.

> I see lots of instances where there's no explicit type-casting to the
> target variable type -
> retval = palloc(sizeof(GISTENTRY));
> Interval   *p = palloc(sizeof(Interval));
> macaddr*v = palloc0(sizeof(macaddr)); and so on.

Yeah.  IMO the first of those is very poor style, because there's
basically nothing enforcing that you wrote the right thing in sizeof().
The others are a bit safer, in that at least a human can note that
the two types mentioned on the same line match --- but I doubt any
compiler would detect it if they don't.  Our current preferred style

 Interval   *p = (Interval *) palloc(sizeof(Interval));

is really barely an improvement, because only two of the three types
mentioned are going to be checked against each other.

So I think Peter's got a good idea here (I might quibble with the details
of some of these macros).  But it's not really going to move the
safety goalposts very far unless we make a concerted effort to make
these be the style everywhere.  Are we willing to do that?  What
will it do to back-patching difficulty?  Dare I suggest back-patching
these changes?

regards, tom lane

Re: Zstandard support for toast compression

[Robert Haas]

On Tue, May 17, 2022 at 12:19 AM Michael Paquier  wrote:
> Toast compression is supported for LZ4, and thanks to the refactoring
> work done with compression methods assigned to an attribute, adding
> support for more methods is straight-forward, as long as we don't
> support more than 4 methods as the compression ID is stored within the
> first 2 bits of the raw length.

Yeah - I think we had better reserve the fourth bit pattern for
something extensible e.g. another byte or several to specify the
actual method, so that we don't have a hard limit of 4 methods. But
even with such a system, the first 3 methods will always and forever
be privileged over all others, so we'd better not make the mistake of
adding something silly as our third algorithm.

I don't particularly have anything against adding Zstandard
compression here, but I wonder whether there's any rush. If we decide
not to add this now, we can always change our minds and add it later,
but if we decide to add it now, there's no backing it out. I'd
probably be inclined to wait and see if our public demands it of us.

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

Re: Zstandard support for toast compression

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, May 17, 2022 at 12:19 AM Michael Paquier  wrote:
> > Toast compression is supported for LZ4, and thanks to the refactoring
> > work done with compression methods assigned to an attribute, adding
> > support for more methods is straight-forward, as long as we don't
> > support more than 4 methods as the compression ID is stored within the
> > first 2 bits of the raw length.
> 
> Yeah - I think we had better reserve the fourth bit pattern for
> something extensible e.g. another byte or several to specify the
> actual method, so that we don't have a hard limit of 4 methods. But
> even with such a system, the first 3 methods will always and forever
> be privileged over all others, so we'd better not make the mistake of
> adding something silly as our third algorithm.

In such a situation, would they really end up being properly distinct
when it comes to what our users see..?  I wouldn't really think so.

> I don't particularly have anything against adding Zstandard
> compression here, but I wonder whether there's any rush. If we decide
> not to add this now, we can always change our minds and add it later,
> but if we decide to add it now, there's no backing it out. I'd
> probably be inclined to wait and see if our public demands it of us.

If anything, this strikes me as a reason to question using a bit for LZ4
and not a mark against Zstd.  Still tho- there seems like a clear path
to having more than 4 when we get demand for it, and here's a patch for
what is pretty clearly one of the better compression methods out there
today.  As another point, while pgbackrest supports gzip, lz4, zstd, and
bzip2, where it's supported, zstd seems to be the most used.  We had
gzip first as zstd wasn't really a proper thing at the time, and lz4 for
speed.  Bzip2 was added more as it was easy to do and of some interest
on systems that didn't have zstd but I wouldn't support adding it to PG
as I'd hope that nearly all systems where v16 is deployed will have Zstd
support.

+1 for adding Zstd for me.

Thanks,

Stephen


signature.asc
Description: PGP signature

Limiting memory allocation

[Stephen Frost]

Greetings,

An ongoing issue in container environments where Kubernetes is being
used is that setting the overcommit parameters on the base system will
impact all of the processes on that system and not all of them handle
malloc failing as gracefully as PG does and may allocate more than what
they really need with the expectation that it'll work.  Folks have
already started hacking around this issue by using things like
LD_PRELOAD'd libraries to put code between palloc and malloc, but that
could cause any malloc to fail and it strikes me that, in an ideal
world, we could give users a way to constrain the total amount of memory
allocated by regular backends (or those where a user is able to control
how much memory is allocated, more generally) while not impacting the
processes which keep the server running.

I wanted to bring this general idea up for discussion here to get
feedback on the concept before going off to write code for it.  Seems
unlikely that it would be a huge amount of code while there's likely to
need to be discussion about how one would configure this and how we
might handle parallel query and such.

Empirically, the LD_PRELOAD hack does, in fact, seem to work, so even a
simple approach would be helpful, but certainly there is an angle to
this where we might eventually allow certain backends (perhaps certain
roles, etc) to allocate more and others to not be allowed to allocate as
much, etc.  Ideally we would look to go with the simple approch first
and then we can contemplate making it more complicated in the future and
not try to accomplish it all in the first pass.  Of course, we should
keep in mind such ideas and try to avoid anything which would preclude
us for adding that flexibility in the future.

Thoughts?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Zstandard support for toast compression

[Tom Lane]

Stephen Frost  writes:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> Yeah - I think we had better reserve the fourth bit pattern for
>> something extensible e.g. another byte or several to specify the
>> actual method, so that we don't have a hard limit of 4 methods. But
>> even with such a system, the first 3 methods will always and forever
>> be privileged over all others, so we'd better not make the mistake of
>> adding something silly as our third algorithm.

> In such a situation, would they really end up being properly distinct
> when it comes to what our users see..?  I wouldn't really think so.

It should be transparent to users, sure, but the point is that the
first three methods will have a storage space advantage over others.
Plus we'd have to do some actual work to create that extension mechanism.

I'm with Robert in that I do not see any urgency to add another method.
The fact that Stephen is already questioning whether LZ4 should have
been added first is not making me any more eager to jump here.
Compression methods come, and they go, and we do not serve anyone's
interest by being early adopters.

regards, tom lane

Re: Limiting memory allocation

[Jan Wieck]

On 5/17/22 15:42, Stephen Frost wrote:

Thoughts?


Yes.

The main and foremost problem is a server that is used for multiple 
services and they behave differently when it comes to memory allocation. 
One service just allocates like we have petabytes of RAM, then uses 
little of it, while another one is doing precise accounting and uses all 
of that. These two types of services don't coexist well on one system 
without intervention.


Unfortunately swap space has been shunned as the ugly stepchild of 
memory in recent years. It could help in this regard to bring back swap 
space, but don't really intend to use it.


Using cgroups one can actually force a certain process (or user, or 
service) to use swap if and when that service is using more memory than 
it was "expected" to use. So I have a server with 64G of RAM. I give 16G 
to Postgres as shared buffers and another 16G to work with. I assume 
another 16G of OS buffers, so I restrict the Apache-Tomcat stuff running 
on it to something like 8-12G. After that, it has to swap. Of course, my 
Postgres processes also will have to swap if they need more than 16G of 
overall workmem ... but that is what I actually intended. I may have to 
reduce workmem, or max_connections, or something else.



Regards, Jan

Re: Zstandard support for toast compression

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> Yeah - I think we had better reserve the fourth bit pattern for
> >> something extensible e.g. another byte or several to specify the
> >> actual method, so that we don't have a hard limit of 4 methods. But
> >> even with such a system, the first 3 methods will always and forever
> >> be privileged over all others, so we'd better not make the mistake of
> >> adding something silly as our third algorithm.
> 
> > In such a situation, would they really end up being properly distinct
> > when it comes to what our users see..?  I wouldn't really think so.
> 
> It should be transparent to users, sure, but the point is that the
> first three methods will have a storage space advantage over others.
> Plus we'd have to do some actual work to create that extension mechanism.
> 
> I'm with Robert in that I do not see any urgency to add another method.
> The fact that Stephen is already questioning whether LZ4 should have
> been added first is not making me any more eager to jump here.
> Compression methods come, and they go, and we do not serve anyone's
> interest by being early adopters.

I'm getting a bit of deja-vu here from when I was first trying to add
TRUNCATE as a GRANT'able option and being told we didn't want to burn
those precious bits.

But, fine, then I'd suggest to Michael that he work on actively solving
the problem we've now got where we have such a limited number of bits,
and then come back and add Zstd after that's done.  I disagree that we
should be pushing back so hard on adding Zstd in general, but if we are
going to demand that we have a way to support more than these few
compression options before ever adding any new ones (considering how
long it's taken Zstd to get to the level it is now, we're talking
about close to a *decade* from such a new algorithm showing up and
getting to a similar level of adoption, and then apparently more because
we don't feel it's 'ready' yet), then let's work towards that and not
complain when it shows up that it's not needed yet (as I fear would
happen ... and just leave us unable to make useful progress).

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Limiting memory allocation

[Tom Lane]

Jan Wieck  writes:
> On 5/17/22 15:42, Stephen Frost wrote:
>> Thoughts?

> Using cgroups one can actually force a certain process (or user, or 
> service) to use swap if and when that service is using more memory than 
> it was "expected" to use.

I wonder if we shouldn't just provide documentation pointing to OS-level
facilities like that one.  The kernel has a pretty trivial way to check
the total memory used by a process.  We don't: it'd require tracking total
space used in all our memory contexts, and then extracting some number out
of our rear ends for allocations made directly from malloc.  In short,
anything we do here will be slow and unreliable, unless you want to depend
on platform-specific things like looking at /proc/self/maps.

ulimit might be interesting to check into as well.  The last time I
looked, it wasn't too helpful for this on Linux, but that was years ago.

regards, tom lane

Re: Limiting memory allocation

[Stephen Frost]

Greetings,

On Tue, May 17, 2022 at 18:12 Tom Lane  wrote:

> Jan Wieck  writes:
> > On 5/17/22 15:42, Stephen Frost wrote:
> >> Thoughts?
>
> > Using cgroups one can actually force a certain process (or user, or
> > service) to use swap if and when that service is using more memory than
> > it was "expected" to use.
>
> I wonder if we shouldn't just provide documentation pointing to OS-level
> facilities like that one.  The kernel has a pretty trivial way to check
> the total memory used by a process.  We don't: it'd require tracking total
> space used in all our memory contexts, and then extracting some number out
> of our rear ends for allocations made directly from malloc.  In short,
> anything we do here will be slow and unreliable, unless you want to depend
> on platform-specific things like looking at /proc/self/maps.


This isn’t actually a solution though and that’s the problem- you end up
using swap but if you use more than “expected” the OOM killer comes in and
happily blows you up anyway. Cgroups are containers and exactly what kube
is doing.

I agree with the general statement that it would be better for the kernel
to do this, and a patch was written for it but then rejected by the kernel
folks. I’m hoping to push on that with the kernel developers but they
seemed pretty against this and that’s quite unfortunate.

As for the performance concern and other mallocs: For the former, thanks to
our memory contexts, I don’t expect it to be all that much of an issue as
the actual allocations we do aren’t all that frequently done and apparently
a relatively trivial implementation was done and performance was tested and
it was claimed that there was basically negligible impact. Sadly that code
isn’t open (yet… this is under discussion, supposedly) but my understanding
was that they just used a simple bit of shared memory to keep the count.
As for the latter, we could at least review the difference between our
count and actual memory allocated and see how big that difference is in
some testing (which might be enlightening anyway..) and review our direct
mallocs and see if there’s a real concern there. Naturally this approach
would necessitate some amount less than the total amount of memory
available being used by PG anyway, but that could certainly be desirable in
some scenarios where there are other processes running and to ensure not
all of the filesystem cache is ejected.

ulimit might be interesting to check into as well.  The last time I
> looked, it wasn't too helpful for this on Linux, but that was years ago.


Unfortunately I really don’t think anything here has materially changed in
a way which would help us.  This would also apply across all of PG’s
processes and I would think it’d be nice to differentiate between user
backends running away and sucking up a ton of memory vs backend processes
that shouldn’t be constrained in this way.

Thanks,

Stephen

>

Re: Limiting memory allocation

[Tom Lane]

Stephen Frost  writes:
> On Tue, May 17, 2022 at 18:12 Tom Lane  wrote:
>> ulimit might be interesting to check into as well.  The last time I
>> looked, it wasn't too helpful for this on Linux, but that was years ago.

> Unfortunately I really don’t think anything here has materially changed in
> a way which would help us.  This would also apply across all of PG’s
> processes and I would think it’d be nice to differentiate between user
> backends running away and sucking up a ton of memory vs backend processes
> that shouldn’t be constrained in this way.

It may well be that they've not fixed its shortcomings, but the claim
that it couldn't be applied selectively is nonsense.  See setrlimit(2),
which we already use successfully (AFAIK) to set stack space on a
per-process basis.

regards, tom lane

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

[Ranier Vilela]

Em ter., 17 de mai. de 2022 às 10:33, Ranier Vilela 
escreveu:

> Em seg., 16 de mai. de 2022 às 20:26, David Rowley 
> escreveu:
>
>> On Sun, 15 May 2022 at 09:47, Ranier Vilela  wrote:
>> > At function load_relcache_init_file, there is an unnecessary function
>> call,
>> > to initialize pgstat_info pointer to NULL.
>> >
>> > MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));
>>
>> What seems to have happened here is the field was changed to become a
>> pointer in 77947c51c.  It's not incorrect to use MemSet() to zero out
>> the pointer field.  What it does probably do is confuse the casual
>> reader into thinking the field is a struct rather than a pointer to
>> one.   It's probably worth making that consistent with the other
>> fields so nobody gets confused.
>>
>> Can you add a CF entry for PG16 for this so we come back to it after we
>> branch?
>>
> Of course.
> I will add it.
>
Created https://commitfest.postgresql.org/38/3640/
However, I would like to add more.
I found, I believe, a serious problem of incorrect usage of the memset api.
Historically, people have relied on using memset or MemSet, using the
variable name as an argument for the sizeof.
While it works correctly, for arrays, when it comes to pointers to
structures, things go awry.

#include 

struct test_t
{
 double b;
 int a;
 char c;
};

typedef struct test_t Test;

int main()
{
 Test * my_test;

 printf("Sizeof pointer=%u\n", sizeof(my_test));
 printf("Sizeof struct=%u\n", sizeof(Test));
}

Output:
Sizeof pointer=8
Sizeof struct=16

So throughout the code there are these misuses.

So, taking advantage of this CF I'm going to add one more big patch, with
suggestions to fix the calls.
This pass vcregress check.

regards,
Ranier Vilela


001-avoid_unecessary_memset_call.patch
Description: Binary data


002-fix_api_memset_usage.patch
Description: Binary data

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

[Justin Pryzby]

On Tue, May 17, 2022 at 07:52:30PM -0300, Ranier Vilela wrote:
> I found, I believe, a serious problem of incorrect usage of the memset api.
> Historically, people have relied on using memset or MemSet, using the
> variable name as an argument for the sizeof.
> While it works correctly, for arrays, when it comes to pointers to
> structures, things go awry.

Knowing how sizeof() works is required before using it - the same is true for
pointers.

> So throughout the code there are these misuses.

Why do you think it's a misuse ?

Take the first one as an example.  It says:

GenericCosts costs;
MemSet(&costs, 0, sizeof(costs));

You sent a patch to change it to sizeof(GenericCosts).

But it's not a pointer, so they are the same.

Is that true for every change in your patch ?

-- 
Justin

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

[Tom Lane]

Ranier Vilela  writes:
> I found, I believe, a serious problem of incorrect usage of the memset api.
> Historically, people have relied on using memset or MemSet, using the
> variable name as an argument for the sizeof.
> While it works correctly, for arrays, when it comes to pointers to
> structures, things go awry.

You'll have to convince people that any of these places are in
fact incorrect.  Everyone who's used C for any length of time
is well aware of the possibility of getting sizeof() wrong in
this sort of context, and I think we've been careful about it.

Also, as a stylistic matter I think it's best to write
"memset(&x, 0, sizeof(x))" where we can.  Replacing sizeof(x)
with sizeof(some type name) has its own risks of error, and
therefore is not automatically an improvement.

regards, tom lane

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

[Ranier Vilela]

Em ter., 17 de mai. de 2022 às 20:18, Justin Pryzby 
escreveu:

> On Tue, May 17, 2022 at 07:52:30PM -0300, Ranier Vilela wrote:
> > I found, I believe, a serious problem of incorrect usage of the memset
> api.
> > Historically, people have relied on using memset or MemSet, using the
> > variable name as an argument for the sizeof.
> > While it works correctly, for arrays, when it comes to pointers to
> > structures, things go awry.
>
> Knowing how sizeof() works is required before using it - the same is true
> for
> pointers.
>
> > So throughout the code there are these misuses.
>
> Why do you think it's a misuse ?
>
> Take the first one as an example.  It says:
>
> GenericCosts costs;
> MemSet(&costs, 0, sizeof(costs));
>
> You sent a patch to change it to sizeof(GenericCosts).
>
> But it's not a pointer, so they are the same.
>
> Is that true for every change in your patch ?
>
It seems true, sorry.
Thanks Justin for pointing out my big mistake.

I hope this isn't all wasted work, but should I remove the 002 patch.

regards,
Ranier Vilela

ALTER TABLE SET ACCESS METHOD on partitioned tables

[Soumyadeep Chakraborty]

Hello,

This is a fresh thread to continue the discussion on ALTER TABLE SET
ACCESS METHOD when applied to partition roots, as requested.

Current behavior (HEAD):

CREATE TABLE am_partitioned(x INT, y INT)
   PARTITION BY hash (x);
ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
ERROR:  cannot change access method of a partitioned table

Potential behavior options:

(A) Don't recurse to existing children and ensure that the new am gets
inherited by any new children. (ALTER TABLE SET TABLESPACE behavior)

(B) Recurse to existing children and modify their am. Also, ensure that
any new children inherit the new am.

A patch [1] was introduced earlier by Justin to implement
(A). v1-0001-Allow-ATSETAM-on-partition-roots.patch contains a rebase
of that patch against latest HEAD, with minor updates on comments and
some additional test coverage.

I think that (B) is necessary for partition hierarchies with a high
number of partitions. One typical use case in Greenplum, for
instance, is to convert heap tables containing cold data to append-only
storage at the root or subroot level of partition hierarchies consisting
of thousands of partitions. Asking users to ALTER individual partitions
is cumbersome and error-prone.

Furthermore, I believe that (B) should be the default and (A) can be
chosen by using the ONLY clause. This would give us the best of both
worlds and would make the use of ONLY consistent. The patch
v1-0002-Make-ATSETAM-recurse-by-default.patch achieves that.

Thoughts?

Regards,
Soumyadeep (VMware)


[1]
https://www.postgresql.org/message-id/20210308010707.GA29832%40telsasoft.com
From 440517ff8a5912d4c657a464b2c1de9c8b3a4f70 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty 
Date: Tue, 17 May 2022 15:22:48 -0700
Subject: [PATCH v1 2/2] Make ATSETAM recurse by default

ATSETAM now recurses to partition children by default. To prevent
recursion, and have the new am apply to future children only, users must
specify the ONLY clause.
---
 src/backend/commands/tablecmds.c|  2 ++
 src/test/regress/expected/create_am.out | 13 -
 src/test/regress/sql/create_am.sql  |  5 -
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3f411dd71f..58251c22ffe 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4711,6 +4711,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 		 errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
 
+			if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
 			ATPrepSetAccessMethod(tab, rel, cmd->name);
 			pass = AT_PASS_MISC;	/* does not matter; no work in Phase 2 */
 			break;
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index a48199df9a2..e99de1ac912 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -284,7 +284,7 @@ CREATE TABLE am_partitioned(x INT, y INT)
 CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
 CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
 ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
 CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
 SELECT relname, amname FROM pg_class c, pg_am am
   WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
@@ -296,6 +296,17 @@ SELECT relname, amname FROM pg_class c, pg_am am
  am_partitioned_3 | heap2
 (4 rows)
 
+ALTER TABLE am_partitioned SET ACCESS METHOD heap;
+SELECT relname, amname FROM pg_class c, pg_am am
+WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ relname  | amname 
+--+
+ am_partitioned   | heap
+ am_partitioned_1 | heap
+ am_partitioned_2 | heap
+ am_partitioned_3 | heap
+(4 rows)
+
 DROP TABLE am_partitioned;
 -- Second, create objects in the new AM by changing the default AM
 BEGIN;
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 8f3db03bba2..f4e22684782 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -184,10 +184,13 @@ CREATE TABLE am_partitioned(x INT, y INT)
 CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
 CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
 ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
 CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (

Re: Remove support for Visual Studio 2013

[Michael Paquier]

On Tue, May 17, 2022 at 06:26:20PM +0200, Juan José Santamaría Flecha wrote:
> This is because current MinGW defaults to Windows 2003 [1], maybe we should
> fix Windows' minimal version to Vista (0x0600) unconditionally also. I have
> seen a couple of compilation warnings while testing that setting on MinGW,
> please find attached a patch for so.
> 
> [1]
> https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sdkddkver.h

Ah, right.  I have forgotten about this business with MinGW.

@@ -1757,7 +1757,7 @@ get_collation_actual_version(char collprovider, const 
char *collcollate)
 collcollate,
 GetLastError(;
 }
-collversion = psprintf("%d.%d,%d.%d",
+collversion = psprintf("%ld.%ld,%ld.%ld",
(version.dwNLSVersion >> 8) & 0x,
version.dwNLSVersion & 0xFF,
(version.dwDefinedVersion >> 8) & 0x,

Is this change still required even if we bump MIN_WINNT to 0x0600 for
all the environments that include win32.h?  At the end, this would
mean dropping support for Windows XP and Windows Server 2003 as
run-time environments as listed in [1], which are not supported
officially since 2014 (even if there have been some patches for
some critical issues).  So I'd be fine to raise the bar for v16~,
particularly as this would allow us to get rid of this code related to
locales.

[1]: 
https://docs.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=msvc-170
--
Michael


signature.asc
Description: PGP signature

Re: pgbench --partitions=0

[Michael Paquier]

On Mon, May 16, 2022 at 03:00:51PM +0900, Michael Paquier wrote:
> (I have added an open item, just in case.)

And fixed as of 27f1366.
--
Michael


signature.asc
Description: PGP signature

Re: amcheck is using a wrong macro to check compressed-ness

[Michael Paquier]

On Tue, May 17, 2022 at 04:58:11PM +0900, Michael Paquier wrote:
> Adding Robert in CC, as this has been added with bd807be.  I have
> added an open item for now.

With the individual in CC, that's even better.
--
Michael


signature.asc
Description: PGP signature

Re: create_help.pl treats as replaceable

[Kyotaro Horiguchi]

At Tue, 17 May 2022 11:09:23 -0400, Tom Lane  wrote in 
> but that just seems flat-out wrong.  If "match" is a keyword it should
> be rendered like other keywords.  I'm not very interested in splitting
> hairs about whether the grammar thinks it is a keyword --- it looks like
> one to a user.  So I think
> 
> HEADER [ boolean | MATCH ]
> 
> would be a better solution.

Oh, agreed. Thanks for the correction. By the way the error message in
defGetCopyHeaderChoice is as follows.

"%s requires a Boolean value or \"match\""

Should it be "%s requires a boolean value or MATCH"?

At least I think "Boolean" should be un-capitalized. The second
attached replaces "Booean" with "boolean" and the \"match\" above to
MATCH.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 8fc4910b57868f8cb2c68d1af31f3d96523e036b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 18 May 2022 09:43:13 +0900
Subject: [PATCH 1/2] Fix copy-from doc

An option keyword "match" is marked as  but it should be bare
upper-cased word. That mistake caused sql_help.c contain an extra
translatable word.
---
 doc/src/sgml/ref/copy.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index e7a6676efd..a3411b4564 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -36,7 +36,7 @@ COPY { table_name [ ( boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
-HEADER [ boolean | match ]
+HEADER [ boolean | MATCH ]
 QUOTE 'quote_character'
 ESCAPE 'escape_character'
 FORCE_QUOTE { ( column_name [, ...] ) | * }
-- 
2.27.0

>From 12e321e909f7ca6a20e2a94ccf47fcdaef77a99a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 18 May 2022 09:50:16 +0900
Subject: [PATCH 2/2] Fix wording convention in error messages

Some error messages wrongly used capitalized "Boolean" in the middle a
sentence.  Also fix a use of quoted lower-cased word for a keyword
"MATCH" to be bare, upper-cased word.
---
 src/backend/commands/copy.c  | 2 +-
 src/backend/commands/define.c| 2 +-
 src/backend/commands/extension.c | 6 +++---
 src/backend/utils/misc/guc.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f448d39c7e..b76c5700ed 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -366,7 +366,7 @@ defGetCopyHeaderChoice(DefElem *def)
 	}
 	ereport(ERROR,
 			(errcode(ERRCODE_SYNTAX_ERROR),
-			 errmsg("%s requires a Boolean value or \"match\"",
+			 errmsg("%s requires a boolean value or MATCH",
 	def->defname)));
 	return COPY_HEADER_FALSE;	/* keep compiler quiet */
 }
diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c
index 0755ab1eae..19abb07614 100644
--- a/src/backend/commands/define.c
+++ b/src/backend/commands/define.c
@@ -151,7 +151,7 @@ defGetBoolean(DefElem *def)
 	}
 	ereport(ERROR,
 			(errcode(ERRCODE_SYNTAX_ERROR),
-			 errmsg("%s requires a Boolean value",
+			 errmsg("%s requires a boolean value",
 	def->defname)));
 	return false;/* keep compiler quiet */
 }
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 767d9b9619..f95ae4964d 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -560,7 +560,7 @@ parse_extension_control_file(ExtensionControlFile *control,
 			if (!parse_bool(item->value, &control->relocatable))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-		 errmsg("parameter \"%s\" requires a Boolean value",
+		 errmsg("parameter \"%s\" requires a boolean value",
 item->name)));
 		}
 		else if (strcmp(item->name, "superuser") == 0)
@@ -568,7 +568,7 @@ parse_extension_control_file(ExtensionControlFile *control,
 			if (!parse_bool(item->value, &control->superuser))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-		 errmsg("parameter \"%s\" requires a Boolean value",
+		 errmsg("parameter \"%s\" requires a boolean value",
 item->name)));
 		}
 		else if (strcmp(item->name, "trusted") == 0)
@@ -576,7 +576,7 @@ parse_extension_control_file(ExtensionControlFile *control,
 			if (!parse_bool(item->value, &control->trusted))
 ereport(ERROR,
 		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-		 errmsg("parameter \"%s\" requires a Boolean value",
+		 errmsg("parameter \"%s\" requires a boolean value",
 item->name)));
 		}
 		else if (strcmp(item->name, "encoding") == 0)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8e9b71375c..0408754e68 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7369,7 +7369,7 @@ parse_and_validate_value(struct config_generic *record,
 {
 	ereport(elevel,
 			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-			 errmsg("parameter \"%s\" requires a Boolean value",
+			 errm

Re: pgbench --partitions=0

[Amit Langote]

On Wed, May 18, 2022 at 9:50 Michael Paquier  wrote:

> On Mon, May 16, 2022 at 03:00:51PM +0900, Michael Paquier wrote:
> > (I have added an open item, just in case.)
>
> And fixed as of 27f1366

Thank you.
-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

funcs.sgml - wrong example

[Erik Rijkers]

funcs.sgml  has

  42 <@ '{[1,7)}'::int4multirange

and calls it true.  The attached fixes that.

Included are two more changes where actual output differs a bit from 
what the doc examples show.


Erik

--- doc/src/sgml/func.sgml.orig	2022-05-17 17:50:40.975410855 +0200
+++ doc/src/sgml/func.sgml	2022-05-17 17:55:22.492016016 +0200
@@ -1046,7 +1046,7 @@


 @ -5.0
-5
+5.0

   
 
@@ -1463,7 +1463,7 @@


log(2.0, 64.0)
-   6.00
+   6.

   
 
@@ -21359,7 +21359,7 @@


 42 <@ '{[1,7)}'::int4multirange
-t
+f

   
 

Re: funcs.sgml - wrong example

[Kyotaro Horiguchi]

At Wed, 18 May 2022 03:08:32 +0200, Erik Rijkers  wrote in 
> funcs.sgml  has
> 
>   42 <@ '{[1,7)}'::int4multirange
> 
> and calls it true.  The attached fixes that.
> 
> Included are two more changes where actual output differs a bit from
> what the doc examples show.

A bit off-topic and just out of curiocity, is there a reason other
than speed (and history?) for that we won't truncate trailing zeros in
the output of log(b,n)?

Since we have get_min_scale since 13, for example, with the following
tweak, we get 6.0 for log(2.0, 64.0), which looks nicer.


@@ -10300,6 +10300,8 @@ log_var(const NumericVar *base, const NumericVar *num, 
NumericVar *result)
/* Divide and round to the required scale */
div_var_fast(&ln_num, &ln_base, result, rscale, true);
 
+   result->dscale = Max(get_min_scale(result), base->dscale);
+   result->dscale = Max(result->dscale, num->dscale);
free_var(&ln_num);

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: funcs.sgml - wrong example

[Kyotaro Horiguchi]

At Wed, 18 May 2022 11:11:02 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> At Wed, 18 May 2022 03:08:32 +0200, Erik Rijkers  wrote in 
> > funcs.sgml  has
> > 
> >   42 <@ '{[1,7)}'::int4multirange
> > 
> > and calls it true.  The attached fixes that.
> > 
> > Included are two more changes where actual output differs a bit from
> > what the doc examples show.

Forgot to mention, the all changes look good.  The log(b,n) has 16
trailing digits at least since 9.6.

> A bit off-topic and just out of curiocity, is there a reason other
> than speed (and history?) for that we won't truncate trailing zeros in
> the output of log(b,n)?

Hmm. A bit wrong. I meant that, if we can allow some additional cycles
and we don't stick to the past behavior of the function, we could have
a nicer result.

> Since we have get_min_scale since 13, for example, with the following
> tweak, we get 6.0 for log(2.0, 64.0), which looks nicer.
> 
> 
> @@ -10300,6 +10300,8 @@ log_var(const NumericVar *base, const NumericVar 
> *num, NumericVar *result)
>   /* Divide and round to the required scale */
>   div_var_fast(&ln_num, &ln_base, result, rscale, true);
>  
> + result->dscale = Max(get_min_scale(result), base->dscale);
> + result->dscale = Max(result->dscale, num->dscale);
>   free_var(&ln_num);

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: bogus: logical replication rows/cols combinations

[Amit Kapila]

On Tue, May 17, 2022 at 2:40 PM houzj.f...@fujitsu.com
 wrote:
>
> Attach the new version patch which addressed all the above comments and
> comments from Shi yu[1] and Osumi-san[2].
>

Thanks, your first patch looks good to me. I'll commit that tomorrow
unless there are more comments on the same. The second one is also in
good shape but I would like to test it a bit more and also see if
others have any suggestions/objections on the same.

-- 
With Regards,
Amit Kapila.

RE: Skipping schema changes in publication

[shiy.f...@fujitsu.com]

On Sat, May 14, 2022 9:33 PM vignesh C  wrote:
> 
> Thanks for the comments, the attached v5 patch has the changes for the
> same. Also I have made the changes for SKIP Table based on the new
> syntax, the changes for the same are available in
> v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.
>

Thanks for your patch. Here are some comments on v5-0001 patch.

+   Oid relid = lfirst_oid(lc);
+
+   prid = GetSysCacheOid2(PUBLICATIONRELMAP, 
Anum_pg_publication_rel_oid,
+  
ObjectIdGetDatum(relid),
+  
ObjectIdGetDatum(pubid));
+   if (!OidIsValid(prid))
+   ereport(ERROR,
+   (errcode(ERRCODE_UNDEFINED_OBJECT),
+errmsg("relation \"%s\" is not part of 
the publication",
+   
RelationGetRelationName(rel;

I think the relation in the error message should be the one whose oid is
"relid", instead of relation "rel".

Besides, I think it might be better not to report an error in this case. If
"prid" is invalid, just ignore this relation. Because in RESET cases, we want to
drop all tables in the publications, and there is no specific table.
(If you agree with that, similarly missing_ok should be set to true when calling
PublicationDropSchemas().)

Regards,
Shi yu

Valgrind mem-check for postgres extension

[Natarajan R]

Hi pg-hackers,

I have written a postgres extension, and know that memory leak check can be
done with valgrind. With the help of postgres_valgrind_wiki
 started postgres server with
the valgrind(as given in the wiki)

valgrind --leak-check=no --gen-suppressions=all \
--suppressions=src/tools/valgrind.supp --time-stamp=yes \
--error-markers=VALGRINDERROR-BEGIN,VALGRIND ERROR-END \
--log-file=$HOME/pg-valgrind/%p.log --trace-children=yes \
postgres --log_line_prefix="%m %p " \
--log_statement=all --shared_buffers=64MB 2>&1 | tee
$HOME/pg-valgrind/postmaster.log

I have few doubts in here,

1. When I run with *--leak-check=full*, I get memory leaks for postgres
functions under possibly or definitely lost categories.. Is this expected?
If yes, how shall i ignore it?(by creating .supp?).. kindly suggest
2. Is there any other way to test my extension memory leaks alone, because
combining with postgres leaks is making instrumentation complex?..
3. I have seen some macros for valgrind support within postgres source code
under utils/memdebug.h, but couldn't get complete idea of using it from the
comments in pg_config_manual.h under *USE_VALGRIND *macro, pls provide some
guidance here..

Thank you,
Natarajan R

Re: Handle infinite recursion in logical replication setup

[Amit Kapila]

On Wed, May 4, 2022 at 12:17 PM vignesh C  wrote:
>
> Thanks for the comments, the attached v13 patch has the changes for the same.
>

Few comments on v13-0001
==
1.
+ *
+ * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
+ * PG16.
...
@@ -477,6 +489,12 @@ pgoutput_startup(LogicalDecodingContext *ctx,
OutputPluginOptions *opt,
  else
  ctx->twophase_opt_given = true;

+ if (data->local_only && data->protocol_version <
LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("requested proto_version=%d does not support local_only, need
%d or higher",
+ data->protocol_version, LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)));

What is the need to change the protocol version for this parameter? As
per my understanding, we only need a different protocol version when
we are sending some new message or some additional information in an
existing message as we have done for the streaming/two_phase options
which doesn't seem to be the case here.

2.
@@ -29,6 +29,7 @@ typedef struct PGOutputData
  bool streaming;
  bool messages;
  bool two_phase;
+ bool local_only;
 } PGOutputData;

It seems we already have a similar option named 'only_local' in
TestDecodingData which has the exactly same functionality. So, isn't
it better to name this as 'only_local' instead of 'local_only'? Also,
let's add a test case for 'only_local' option of test_decoding.

-- 
With Regards,
Amit Kapila.

Re: Valgrind mem-check for postgres extension

[Tom Lane]

Natarajan R  writes:
> I have few doubts in here,

> 1. When I run with *--leak-check=full*, I get memory leaks for postgres
> functions under possibly or definitely lost categories.. Is this expected?

Maybe ... you did not show your test case, so it's hard to say.  But it
could well be that this is an artifact of failing to define USE_VALGRIND.

> 2. Is there any other way to test my extension memory leaks alone, because
> combining with postgres leaks is making instrumentation complex?..

No, not really.

> 3. I have seen some macros for valgrind support within postgres source code
> under utils/memdebug.h, but couldn't get complete idea of using it from the
> comments in pg_config_manual.h under *USE_VALGRIND *macro, pls provide some
> guidance here..

If you didn't build the core code with USE_VALGRIND defined, then none of
this stuff is going to work ideally.

The way I like to do it is to run configure, and then manually add
"#define USE_VALGRIND" to the generated src/include/pg_config.h
file before invoking "make".  Probably other people have different
habits.

regards, tom lane

Re: Zstandard support for toast compression

[Michael Paquier]

On Tue, May 17, 2022 at 04:12:14PM -0400, Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I'm with Robert in that I do not see any urgency to add another method.

Okay.

>> The fact that Stephen is already questioning whether LZ4 should have
>> been added first is not making me any more eager to jump here.
>> Compression methods come, and they go, and we do not serve anyone's
>> interest by being early adopters.

FWIW, I don't really question the choice of LZ4 as an alternative to
pglz.  One very easily outclasses the other, guess which one.  Perhaps
we would have gone with zstd back in the day, but here we are, andx
this option is already very good in itself.

Zstandard may not be old enough to vote, being only 7, but its use is
already quite spread.  So I would not be surprised if it remains
popular for many years.  We'll see how it goes.

> But, fine, then I'd suggest to Michael that he work on actively solving
> the problem we've now got where we have such a limited number of bits,
> and then come back and add Zstd after that's done.  I disagree that we
> should be pushing back so hard on adding Zstd in general, but if we are
> going to demand that we have a way to support more than these few
> compression options before ever adding any new ones (considering how
> long it's taken Zstd to get to the level it is now, we're talking
> about close to a *decade* from such a new algorithm showing up and
> getting to a similar level of adoption, and then apparently more because
> we don't feel it's 'ready' yet), then let's work towards that and not
> complain when it shows up that it's not needed yet (as I fear would
> happen ... and just leave us unable to make useful progress).

Saying that, I agree with the point to not set in stone the 4th bit
used in the toast compression header, and that is would be better to
use it for a more extensible design.  Didn't the proposal to introduce
the custom compression mechanisms actually touch this area?  The set
of macros we have currently for the toast values in a varlena are
already kind of hard to figure out.  Making that harder to parse would
not be appealing, definitely.
--
Michael


signature.asc
Description: PGP signature