Re: Ignoring BRIN for HOT udpates seems broken

[Tomas Vondra]

On 5/28/22 21:24, Matthias van de Meent wrote:
> On Sat, 28 May 2022 at 16:51, Tomas Vondra
>  wrote:
>>
>> Hi,
>>
>> while working on some BRIN stuff, I realized (my) commit 5753d4ee320b
>> ignoring BRIN indexes for HOT is likely broken. Consider this example:
>>
>> --
>> CREATE TABLE t (a INT) WITH (fillfactor = 10);
>>
>> INSERT INTO t SELECT i
>>   FROM generate_series(0,10) s(i);
>>
>> CREATE INDEX ON t USING BRIN (a);
>>
>> UPDATE t SET a = 0 WHERE random() < 0.01;
>>
>> SET enable_seqscan = off;
>> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
>>
>> SET enable_seqscan = on;
>> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
>> --
>>
>> which unfortunately produces this:
>>
>>   QUERY PLAN
>> ---
>>  Bitmap Heap Scan on t (actual rows=23 loops=1)
>>Recheck Cond: (a = 0)
>>Rows Removed by Index Recheck: 2793
>>Heap Blocks: lossy=128
>>->  Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1)
>>  Index Cond: (a = 0)
>>  Planning Time: 0.049 ms
>>  Execution Time: 0.424 ms
>> (8 rows)
>>
>> SET
>>QUERY PLAN
>> -
>>  Seq Scan on t (actual rows=995 loops=1)
>>Filter: (a = 0)
>>Rows Removed by Filter: 99006
>>  Planning Time: 0.027 ms
>>  Execution Time: 7.670 ms
>> (5 rows)
>>
>> That is, the index fails to return some of the rows :-(
>>
>> I don't remember the exact reasoning behind the commit, but the commit
>> message justifies the change like this:
>>
>> There are no index pointers to individual tuples in BRIN, and the
>> page range summary will be updated anyway as it relies on visibility
>> info.
>>
>> AFAICS that's a misunderstanding of how BRIN uses visibility map, or
>> rather does not use. In particular, bringetbitmap() does not look at the
>> vm at all, so it'll produce incomplete bitmap.
>>
>> So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a
>> live issue. I don't quite see if this can be salvaged - I'll think about
>> this a bit more, but it'll probably end with a revert.
> 
> The principle of 'amhotblocking' for only blocking HOT updates seems
> correct, except for the fact that the HOT flag bit is also used as a
> way to block the propagation of new values to existing indexes.
> 
> A better abstraction would be "amSummarizes[Block]', in which updates
> that only modify columns that are only included in summarizing indexes
> still allow HOT, but still will see an update call to all (relevant?)
> summarizing indexes. That should still improve performance
> significantly for the relevant cases.
> 

Yeah, I think that might/should work. We could still create the HOT
chain, but we'd have to update the BRIN indexes. But that seems like a
fairly complicated change to be done this late for PG15.

regards

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

Re: fix stats_fetch_consistency value in postgresql.conf.sample

[Andres Freund]

Hi,

On 2022-05-26 16:27:53 +0900, Kyotaro Horiguchi wrote:
> It could be in SQL, but *I* prefer to use perl for this, since it
> allows me to write a bit complex things (than simple string
> comparison) simpler.

I wonder if we shouldn't just expose a C function to do this, rather than
having a separate implementation in a tap test.


> +# parameter names that cannot get consistency check performed
> +my @ignored_parameters =

I think most of these we could ignore by relying on source <> 'override'
instead of listing them?


> +# parameter names that requires case-insensitive check
> +my @case_insensitive_params =
> +  ('ssl_ciphers',
> +   'log_filename',
> +   'event_source',
> +   'log_timezone',
> +   'timezone',
> +   'lc_monetary',
> +   'lc_numeric',
> +   'lc_time');

Why do these differ by case?

Greetings,

Andres Freund

Re: fix stats_fetch_consistency value in postgresql.conf.sample

[Andres Freund]

Hi,

On 2022-05-25 14:00:23 +0900, Michael Paquier wrote:
> On Tue, May 24, 2022 at 09:28:49PM -0700, Andres Freund wrote:
> > And done. Thanks Nathan!
>
> Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for
> consistency?

Yes. Now that the visible sheen of embarassment in my face has subsided a bit
(and pgcon has ended), I pushed this bit too.

- Andres

Re: pg_upgrade test writes to source directory

[Tom Lane]

Andres Freund  writes:
> I suspect there might be a bit more polish might be needed - that's why I
> hadn't proposed the commit on its own yet.

Yeah, I'd noticed the obsoleted comments too, but not bothered to complain
since that was just WIP and not an officially proposed patch.  I'll be
happy to review if you want to put up a full patch.

> I was also wondering about
> proposing a different split (test data, test logs).

Might be too invasive for back-patch.

regards, tom lane

Re: pg_upgrade test writes to source directory

[Andres Freund]

Hi,

On 2022-05-27 09:05:43 +0900, Michael Paquier wrote:
> On Thu, May 26, 2022 at 07:51:08PM -0400, Tom Lane wrote:
> > I wouldn't object to doing that, and even back-patching.  It looked
> > like a pretty sane change, and we've learned before that skimping on
> > back-branch test infrastructure is a poor tradeoff.
> 
> Okay, fine by me.  Andres, what do you think about backpatching [1]?
> 
> [1]: 
> https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c

Well, committing and backpatching ;)

I suspect there might be a bit more polish might be needed - that's why I
hadn't proposed the commit on its own yet. I was also wondering about
proposing a different split (test data, test logs).

I don't even know if we still need TESTDIR - since f4ce6c4d3a3 we add the
build dir to PATH, which IIUC was the reason for TESTDIR previously. Afaics
after f4ce6c4d3a3 and the TESTOUTDIR split the only TESTDIR use is in
src/tools/msvc/ecpg_regression.proj - so we could at least restrict it to
that.


Stuff I noticed on a quick skim:

> # In a VPATH build, we'll be started in the source directory, but we want
> # to run in the build directory so that we can use relative paths to
> # access the tmp_check subdirectory; otherwise the output from filename
> # completion tests is too variable.

Just needs a bit of rephrasing.


>   # Determine output directories, and create them.  The base path is the
>   # TESTDIR environment variable, which is normally set by the invoking
>   # Makefile.
>   $tmp_check = $ENV{TESTOUTDIR} ? "$ENV{TESTOUTDIR}" : "tmp_check";
>   $log_path = "$tmp_check/log";

Probably just needs a s/TESTDIR/TESTOUTDIR/


Greetings,

Andres Freund

Re: Compare variables of composite type with slightly different column types

[Andrey Lepikhov]

On 26/5/2022 14:25, Andrey Lepikhov wrote:
I guess, here the compatible_oper() routine can be used to find a 
appropriate operator, or something like that can be invented.

I looked into the 2cd7084 and a4424c5, but don't found any rationale.


In accordance to this idea I prepared a code. For a demo only.

--
regards,
Andrey Lepikhov
Postgres ProfessionalFrom 71cf1a9148b20921aa60d7ae2062620c515557f8 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" 
Date: Sat, 28 May 2022 22:01:01 +0300
Subject: [PATCH] Try to search compatible operator if columns of compared rows
 is differ.

---
 src/backend/utils/adt/rowtypes.c   | 36 +++---
 src/test/regress/expected/rowtypes.out | 11 +---
 src/test/regress/sql/rowtypes.sql  |  2 +-
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index db843a0fbf..432b44ec8c 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -18,14 +18,17 @@
 
 #include "access/detoast.h"
 #include "access/htup_details.h"
+#include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
 #include "common/hashfn.h"
 #include "funcapi.h"
 #include "libpq/pqformat.h"
 #include "miscadmin.h"
+#include "parser/parse_oper.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
 #include "utils/lsyscache.h"
+#include "utils/syscache.h"
 #include "utils/typcache.h"
 
 
@@ -1071,6 +1074,8 @@ record_eq(PG_FUNCTION_ARGS)
int i1;
int i2;
int j;
+   Oid op_func = InvalidOid;
+   FmgrInfoop_func_finfo;
 
check_stack_depth();/* recurses for record-type columns */
 
@@ -1173,12 +1178,27 @@ record_eq(PG_FUNCTION_ARGS)
 * Have two matching columns, they must be same type
 */
if (att1->atttypid != att2->atttypid)
-   ereport(ERROR,
+   {
+   Operator op;
+
+   op = compatible_oper(NULL, list_make1(makeString("=")),
+
att1->atttypid, att2->atttypid, false, -1);
+
+   if (!op)
+   ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
 errmsg("cannot compare dissimilar 
column types %s and %s at record column %d",

format_type_be(att1->atttypid),

format_type_be(att2->atttypid),
j + 1)));
+   else
+   {
+   op_func = oprfuncid(op);
+   fmgr_info(op_func, _func_finfo);
+   ReleaseSysCache(op);
+   }
+
+   }
 
/*
 * If they're not same collation, we don't complain here, but 
the
@@ -1217,8 +1237,18 @@ record_eq(PG_FUNCTION_ARGS)
}
 
/* Compare the pair of elements */
-   InitFunctionCallInfoData(*locfcinfo, 
>eq_opr_finfo, 2,
-
collation, NULL, NULL);
+   if (!OidIsValid(op_func))
+   /* Compare the pair of elements */
+   InitFunctionCallInfoData(*locfcinfo, 
>eq_opr_finfo, 2,
+   
 collation, NULL, NULL);
+   else
+   {
+   /* XXX: Some collation matching logic needed. */
+   collation = (collation != InvalidOid) ? 
collation : DEFAULT_COLLATION_OID;
+   InitFunctionCallInfoData(*locfcinfo, 
_func_finfo, 2,
+   
 collation, NULL, NULL);
+   }
+
locfcinfo->args[0].value = values1[i1];
locfcinfo->args[0].isnull = false;
locfcinfo->args[1].value = values2[i2];
diff --git a/src/test/regress/expected/rowtypes.out 
b/src/test/regress/expected/rowtypes.out
index a4cc2d8c12..8a3c4e9454 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -423,8 +423,12 @@ select a,b from test_table where (a,b) > ('a','a') order 
by a,b;
 
 reset enable_sort;
 -- Check row comparisons with IN
-select * from int8_tbl i8 where i8 in (row(123,456));  -- fail, type mismatch
-ERROR:  cannot compare dissimilar column types bigint and integer at record 
column 1
+select * from int8_tbl i8 

Re: Ignoring BRIN for HOT udpates seems broken

[Matthias van de Meent]

On Sat, 28 May 2022 at 16:51, Tomas Vondra
 wrote:
>
> Hi,
>
> while working on some BRIN stuff, I realized (my) commit 5753d4ee320b
> ignoring BRIN indexes for HOT is likely broken. Consider this example:
>
> --
> CREATE TABLE t (a INT) WITH (fillfactor = 10);
>
> INSERT INTO t SELECT i
>   FROM generate_series(0,10) s(i);
>
> CREATE INDEX ON t USING BRIN (a);
>
> UPDATE t SET a = 0 WHERE random() < 0.01;
>
> SET enable_seqscan = off;
> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
>
> SET enable_seqscan = on;
> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
> --
>
> which unfortunately produces this:
>
>   QUERY PLAN
> ---
>  Bitmap Heap Scan on t (actual rows=23 loops=1)
>Recheck Cond: (a = 0)
>Rows Removed by Index Recheck: 2793
>Heap Blocks: lossy=128
>->  Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1)
>  Index Cond: (a = 0)
>  Planning Time: 0.049 ms
>  Execution Time: 0.424 ms
> (8 rows)
>
> SET
>QUERY PLAN
> -
>  Seq Scan on t (actual rows=995 loops=1)
>Filter: (a = 0)
>Rows Removed by Filter: 99006
>  Planning Time: 0.027 ms
>  Execution Time: 7.670 ms
> (5 rows)
>
> That is, the index fails to return some of the rows :-(
>
> I don't remember the exact reasoning behind the commit, but the commit
> message justifies the change like this:
>
> There are no index pointers to individual tuples in BRIN, and the
> page range summary will be updated anyway as it relies on visibility
> info.
>
> AFAICS that's a misunderstanding of how BRIN uses visibility map, or
> rather does not use. In particular, bringetbitmap() does not look at the
> vm at all, so it'll produce incomplete bitmap.
>
> So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a
> live issue. I don't quite see if this can be salvaged - I'll think about
> this a bit more, but it'll probably end with a revert.

The principle of 'amhotblocking' for only blocking HOT updates seems
correct, except for the fact that the HOT flag bit is also used as a
way to block the propagation of new values to existing indexes.

A better abstraction would be "amSummarizes[Block]', in which updates
that only modify columns that are only included in summarizing indexes
still allow HOT, but still will see an update call to all (relevant?)
summarizing indexes. That should still improve performance
significantly for the relevant cases.

-Matthias

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2022-05-26 11:47:13 +0300, Aleksander Alekseev wrote:
> Thanks for working on this! I'm very enthusiastic about this effort and I was
> glad to see on PGCon Unconference that the majority of the community seems
> to be as well.
> 
> > The halfway decent list includes, I think:
> > 1) cmake
> > 2) bazel
> > 3) meson
> 
> Was SCons considered as an option?
> What pros and cons do you see that make Meson a better choice?

I looked at it and quickly discarded it. From what I could see there's not
been meaningful moves to it in the last couple years, if anything adoption has
been dropping. And I don't think we want to end up relying on yet another half
maintained tool.

Not having a ninja backend etc didn't strike me as great either - the builds
with scons I've done weren't fast at all.

Greetings,

Andres Freund

Re: pg_upgrade test writes to source directory

[Peter Eisentraut]

On 28.05.22 10:56, Michael Paquier wrote:

On Fri, May 27, 2022 at 02:45:57PM +0200, Peter Eisentraut wrote:

I think you can just chdir to ${PostgreSQL::Test::Utils::tmp_check}.


Hmm.  I think that I prefer your initial suggestion with TESTOUTDIR.
This sticks better in the long term, while making things consistent
with 010_tab_completion.pl, the only test that moves to TESTDIR while
running.  So my vote would be to backpatch first the addition of
TESTOUTDIR, then fix the TAP test of pg_upgrade on HEAD to do the
same.


I think it's a bit premature to talk about backpatching, since the patch 
in question hasn't been committed anywhere yet, and AFAICT hasn't even 
really been reviewed yet.


If you want to go this direction, I suggest you extract the patch and 
present it here on its own merit. -- But then I might ask why such a 
broad change post beta when apparently a one-line change would also work.

Re: CREATE COLLATION must be specified

[Peter Eisentraut]

On 28.05.22 20:16, Shay Rojansky wrote:

CREATE COLLATION some_collation (LC_COLLATE = 'en-u-ks-primary',
     LC_CTYPE = 'en-u-ks-primary',
     PROVIDER = icu,
     DETERMINISTIC = False
);

This works on PG14, but on PG15 it errors with 'parameter "locale" must 
be specified'.


I wanted to make sure this breaking change is intentional (it doesn't 
seem documented in the release notes or in the docs for CREATE COLLATION).


This change is intentional, but the documentation could be improved.

CREATE COLLATION must be specified

[Shay Rojansky]

Hi all,

Testing on the PG15 beta, I'm getting new failures when trying to create a
collation:

CREATE COLLATION some_collation (LC_COLLATE = 'en-u-ks-primary',
LC_CTYPE = 'en-u-ks-primary',
PROVIDER = icu,
DETERMINISTIC = False
);

This works on PG14, but on PG15 it errors with 'parameter "locale" must be
specified'.

I wanted to make sure this breaking change is intentional (it doesn't seem
documented in the release notes or in the docs for CREATE COLLATION).

Shay

Re: ICU_LOCALE set database default icu collation but not working as intended.

[Daniel Verite]

jian he wrote:

>   - dbicu3, ICU_LOCALE 'en-u-kr-latn-digit-kf-upper-kn-true'  seems
>   'kf-upper' not grouped strings beginning with character 'A' together?

You seem to expect that the sort algorithm takes characters
from left to right, and when it compares 'A' and 'a', it will
sort the string with the 'A' before, no matter what other
characters are in the rest of the string.

I don't think that's what kf-upper does. I think kf-upper kicks in
only for strings that are identical at the secondary level.
In your example, its effect is to make 'A 70' sort before
'a 70' . The other strings are unaffected.

>   - dbicu4, ICU_LOCALE 'en-u-kr-latn-digit-kn-true'  since upper/lower not
>   explicitly mentioned, and since the collation is deterministic, so
>   character  'A' should be grouped together first then do the numeric value

The deterministic property is only relevant when strings are compared equal
by ICU. Since your collations use the default strength setting (tertiary)
and the strings in your example are all different at this level,
the fact that the collation is deterministic does not play a role in
the results.

Besides, the TR35 doc says for "kn" (numeric ordering)

  "If set to on, any sequence of Decimal Digits (General_Category = Nd in
   the [UAX44]) is sorted at a primary level with its numeric value"

which means that the order of numbers (7, 11, 19, 70, 117) is "stronger"
(primary level) than the relative order of the 'a' and 'A' 
(case difference=secondary level) that precede them.
That's why these numbers drive the sort for these strings that are
otherwise identical at the primary level.


Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Re: postgres and initdb not working inside docker

[Feike Steenbergen]

On Sat, May 28, 2022, 18:35 Roffild  wrote:

> But the volume mount has a limitation with chmod 755. I don't want to
> write the database directly to the container.

Using a $PGDATA subdirectory in a mounted Volume allows you to run with 0700
and also retain this limitation you mention. I don't believe this
limitation is a limitation
of Docker - AFAIK Docker uses the permissions from the Host Directory for
the Mount.

In my experience we have been using (since 2014?)  a subdirectory of the
mounted Volume
and run a statement similar to this on startup of your container, before
starting postgres/initdb or the like

install -o postgres -g postgres -d -m 0700 "${PGDATA}"

> The world has changed! And the old standards don't work...

There's enough people running Postgres in Docker containers in production
for almost a decade.
It does work!

Kind regards,

Feike Steenbergen

Re: postgres and initdb not working inside docker

[David G. Johnston]

On Sat, May 28, 2022 at 9:35 AM Roffild  wrote:

> Docker is now the DevOps standard. It's easier to build an image for
> Docker and run the site with one command.
>
> But the volume mount has a limitation with chmod 755. I don't want to
> write the database directly to the container.
>
> The container is isolated from everything. Therefore, checking the file
> permissions inside the container is meaningless. And writing to the
> container is also a "security hole".
>
> The world has changed! And the old standards don't work...
>
>
Given the general lack of clamoring for this kind of change I'd be more
inclined to believe that your specific attempt at doing this is problematic
rather than there being a pervasive incompatibility between Docker and
PostgreSQL.  There is a host environment, a container environment, multiple
ways to expose host resources to the container, and the command line and/or
docker file configuration itself.  None of which you've shared.  So I think
that skepticism about your claims is quite understandable.

My suspicion is you aren't leveraging named volumes to separate the
container and storage and that doing so will give you the desired
separation and control of the directory permissions.

Based upon my reading of:

https://github.com/docker-library/docs/blob/master/postgres/README.md

and limited personal experience using Docker, I'm inclined to believe it
can be made to work even if you cannot do it exactly the way you are trying
right now.  Absent a use case for why one way is preferable to another
having the bar set at "it works if you do it like this" seems reasonable.

David J.

Re: postgres and initdb not working inside docker

[Roffild]

Docker is now the DevOps standard. It's easier to build an image for 
Docker and run the site with one command.


But the volume mount has a limitation with chmod 755. I don't want to 
write the database directly to the container.


The container is isolated from everything. Therefore, checking the file 
permissions inside the container is meaningless. And writing to the 
container is also a "security hole".


The world has changed! And the old standards don't work...

28.05.2022 18:49, Tom Lane:

Lacks documentation, too.  But it doesn't matter, because we are not
going to accept such a "feature".  The OP has offered no justification
why this is necessary (and no, he's not the first who's ever used
Postgres inside Docker).  Introducing a security hole that goes
against twenty-five years of deliberate project policy is going to
require a heck of a lot better-reasoned argument than "there's an
issue inside Docker".

Re: postgres and initdb not working inside docker

[Daniel Gustafsson]

> On 28 May 2022, at 17:49, Tom Lane  wrote:
> 
> Daniel Gustafsson  writes:
>>> On 28 May 2022, at 14:59, Roffild  wrote:
>>> This patch fixes an issue inside Docker and will not affect other builds.
> 
>> Looks like you generated the patch backwards, it's removing the lines you
>> propose to add.
> 
> Lacks documentation, too.  But it doesn't matter, because we are not
> going to accept such a "feature".  The OP has offered no justification
> why this is necessary (and no, he's not the first who's ever used
> Postgres inside Docker).  Introducing a security hole that goes
> against twenty-five years of deliberate project policy is going to
> require a heck of a lot better-reasoned argument than "there's an
> issue inside Docker".

FWIW, I 100% agree.

--
Daniel Gustafsson   https://vmware.com/

Re: postgres and initdb not working inside docker

[Tom Lane]

Daniel Gustafsson  writes:
>> On 28 May 2022, at 14:59, Roffild  wrote:
>> This patch fixes an issue inside Docker and will not affect other builds.

> Looks like you generated the patch backwards, it's removing the lines you
> propose to add.

Lacks documentation, too.  But it doesn't matter, because we are not
going to accept such a "feature".  The OP has offered no justification
why this is necessary (and no, he's not the first who's ever used
Postgres inside Docker).  Introducing a security hole that goes
against twenty-five years of deliberate project policy is going to
require a heck of a lot better-reasoned argument than "there's an
issue inside Docker".

regards, tom lane

Re: postgres and initdb not working inside docker

[Roffild]

Fix


Looks like you generated the patch backwards, it's removing the lines you
propose to add.diff --git a/configure.ac b/configure.ac
index d093fb88dd..3f0077696b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -749,6 +749,14 @@ PGAC_ARG_BOOL(enable, cassert, no, [enable assertion 
checks (for debugging)],
  [Define to 1 to build with assertion checks. 
(--enable-cassert)])])
 
 
+#
+# Disable file permission checks
+#
+PGAC_ARG_BOOL(enable, check-permissions, yes, [disable file permission checks 
(for Docker)],
+  [AC_DEFINE([ENABLE_CHECK_PERMISSIONS], 1,
+ [Define to 1 to build with permission checks. 
(--disable-check-permissions)])])
+
+
 #
 # Include directories
 #
diff --git a/src/backend/utils/init/miscinit.c 
b/src/backend/utils/init/miscinit.c
index ec6a61594a..bcd56cc7cb 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -351,7 +351,7 @@ checkDataDir(void)
 *
 * XXX can we safely enable this check on Windows?
 */
-#if !defined(WIN32) && !defined(__CYGWIN__)
+#if defined(ENABLE_CHECK_PERMISSIONS) && !defined(WIN32) && 
!defined(__CYGWIN__)
if (stat_buf.st_uid != geteuid())
ereport(FATAL,

(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -371,7 +371,7 @@ checkDataDir(void)
 * be proper support for Unix-y file permissions.  Need to think of a
 * reasonable check to apply on Windows.
 */
-#if !defined(WIN32) && !defined(__CYGWIN__)
+#if defined(ENABLE_CHECK_PERMISSIONS) && !defined(WIN32) && 
!defined(__CYGWIN__)
if (stat_buf.st_mode & PG_MODE_MASK_GROUP)
ereport(FATAL,

(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index cdd742cb55..df44393855 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -45,6 +45,10 @@
 /* Define to the file name extension of dynamically-loadable modules. */
 #undef DLSUFFIX
 
+/* Define to 1 to build with permission checks. (--disable-check-permissions)
+   */
+#undef ENABLE_CHECK_PERMISSIONS
+
 /* Define to build with GSSAPI support. (--with-gssapi) */
 #undef ENABLE_GSS
 

CI and test improvements

[Justin Pryzby]

I'm "joining" a bunch of unresolved threads hoping to present them better since
they're related and I'm maintaining them together anyway.

https://www.postgresql.org/message-id/flat/20220219234148.GC9008%40telsasoft.com
 - set TESTDIR from perl rather than Makefile
https://www.postgresql.org/message-id/flat/20220522232606.GZ19626%40telsasoft.com
 - ccache, MSVC, and meson
https://www.postgresql.org/message-id/20220416144454.GX26620%40telsasoft.com
 - Re: convert libpq uri-regress tests to tap test
https://www.postgresql.org/message-id/CA%2BhUKGLneD%2Bq%2BE7upHGwn41KGvbxhsKbJ%2BM-y9nvv7_Xjv8Qog%40mail.gmail.com
 - Re: A test for replay of regression tests
https://www.postgresql.org/message-id/flat/20220409021853.gp24...@telsasoft.com
 - cfbot requests

See the commit messages for more thread references.

I'm anticipating the need to further re-arrange the patch set - it's not clear
which patches should go first.  Maybe some patches should be dropped in favour
of the meson project.  I guess some patches will have to be re-implemented with
meson (msvc warnings).

I think there was some confusion about the vcregress "alltaptests" target.
I said that it's okay to add it and make cirrus use it (and that the buildfarm
could use it too).  Andrew responded that the buildfarm wants to run different
tests separately.  But Andres seems to have interpretted that as an objection
to the addition of an "alltaptests" target, which I think isn't what's
intended - it's fine if the buildfarm prefers not to use it.

Maybe the improvements to vcregress should go into v15 ?  CI should run all the
tests (which also serves to document *how* to run all the tests, even if there
isn't a literal check-world target).
>From 3d1c842a7210f5595c9737af7cb4349242413e72 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 25 May 2022 21:53:22 -0500
Subject: [PATCH 01/19] cirrus/windows: add compiler_warnings_script

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

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

See also: 8a1ce5e54f6d144e4f8e19af7c767b026ee0c956

ci-os-only: windows

https://cirrus-ci.com/task/6183879907213312
https://cirrus-ci.com/task/4876271443247104
---
 .cirrus.yml|  8 +++-
 src/tools/ci/windows-compiler-warnings | 16 
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/windows-compiler-warnings

diff --git a/.cirrus.yml b/.cirrus.yml
index f23d6cae552..bcb8d53db78 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -370,7 +370,8 @@ task:
 # ForceNoAlign prevents msbuild from introducing line-breaks for long lines
 # disable file tracker, we're never going to rebuild, and it slows down the
 #   build
-MSBFLAGS: -m -verbosity:minimal "-consoleLoggerParameters:Summary;ForceNoAlign" /p:TrackFileAccess=false -nologo
+# -fileLoggerParameters1: write warnings to msbuild.warn.log.
+MSBFLAGS: -m -verbosity:minimal "-consoleLoggerParameters:Summary;ForceNoAlign" /p:TrackFileAccess=false -nologo -fileLoggerParameters1:warningsonly;logfile=msbuild.warn.log
 
 # If tests hang forever, cirrus eventually times out. In that case log
 # output etc is not uploaded, making the problem hard to debug. Of course
@@ -450,6 +451,11 @@ task:
 cd src/tools/msvc
 %T_C% perl vcregress.pl ecpgcheck
 
+  # These should be last, so all the important checks are always run
+  always:
+compiler_warnings_script:
+  - sh src\tools\ci\windows-compiler-warnings msbuild.warn.log
+
   on_failure:
 <<: *on_failure
 crashlog_artifacts:
diff --git a/src/tools/ci/windows-compiler-warnings b/src/tools/ci/windows-compiler-warnings
new file mode 100755
index 000..d6f9a1fc569
--- /dev/null
+++ b/src/tools/ci/windows-compiler-warnings
@@ -0,0 +1,16 @@
+#! /bin/sh
+# Success if the given file doesn't exist or is empty, else fail
+# This is a separate file only to avoid dealing with windows shell quoting and escaping.
+set -e
+
+fn=$1
+
+if [ -s "$fn" ]
+then
+	# Display the file's content, then exit indicating failure
+	cat "$fn"
+	exit 1
+else
+	# Success
+	exit 0
+fi
-- 
2.17.1

>From 5769729262fce53b6d6448016cef5c6351528f8b Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sun, 9 Jan 2022 18:25:02 -0600
Subject: [PATCH 02/19] cirrus/vcregress: test modules/contrib with
 NO_INSTALLCHECK=1

https://www.postgresql.org/message-id/20220109191649.GL14051%40telsasoft.com
https://www.postgresql.org/message-id/CA%2BhUKGLneD%2Bq%2BE7upHGwn41KGvbxhsKbJ%2BM-y9nvv7_Xjv8Qog%40mail.gmail.com
---
 .cirrus.yml|  4 +-
 contrib/basic_archive/Makefile |  2 +-
 contrib/pg_stat_statements/Makefile|  2 +-
 contrib/test_decoding/Makefile |  2 +-
 src/test/modules/snapshot_too_old/Makefile |  2 +-
 src/test/modules/worker_spi/Makefile  

Re: [PATCH] Support % wildcard in extension upgrade filenames

[Tom Lane]

Laurenz Albe  writes:
> I don't think this idea is fundamentally wrong, but I have two worries:

> 1. It would be a good idea good to make sure that there is not both
>"extension--%--2.0.sql" and "extension--1.0--2.0.sql" present.
>Otherwise the behavior might be indeterministic.

The existing logic to find multi-step upgrade paths is going to make
this a very pressing problem.  For example, if you provide both
extension--%--2.0.sql and extension--%--2.1.sql, it's not at all
clear whether the code would try to use both of those or just one
to get from 1.x to 2.1.

> 2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade
>their PostGIS 1.1 installation with it?  Would that work?
>Having a lower bound for a matching version might be a good idea,
>although I have no idea how to do that.

The lack of upper bound is a problem too: what stops the system from
trying to use this to get from (say) 4.2 to 3.3, and if it does try that,
will the script produce a sane result?  (Seems unlikely, as it couldn't
know what 4.x objects it ought to alter or remove.)  But it's going to be
very hard to provide bounds, because we intentionally designed the
extension system to not have an explicit concept of some versions being
less than others.

I'm frankly skeptical that this is a good idea at all.  It seems
to have come out of someone's willful refusal to use the system as
designed, ie as a series of small upgrade scripts that each do just
one step.  I don't feel an urgent need to cater to the
one-monster-script-that-handles-all-cases approach, because no one
has offered any evidence that that's really a better way.  How would
you even write the conditional logic needed ... plpgsql DO blocks?
Testing what?  IIRC we don't expose any explicit knowledge of the
old extension version number to the script.

regards, tom lane

Re: Bump MIN_WINNT to 0x0600 (Vista) as minimal runtime in 16~

[Daniel Gustafsson]

> On 27 May 2022, at 23:07, Thomas Munro  wrote:

> We should go full Marie Kondo on EOL'd OSes that are not in our CI or
> build farm, IMHO.

FWIW, +1

--
Daniel Gustafsson   https://vmware.com/

Re: [PATCH] Support % wildcard in extension upgrade filenames

[Daniel Gustafsson]

> On 28 May 2022, at 16:50, Laurenz Albe  wrote:

> I don't think this idea is fundamentally wrong, but I have two worries:
> 
> 1. It would be a good idea good to make sure that there is not both
> "extension--%--2.0.sql" and "extension--1.0--2.0.sql" present.
> Otherwise the behavior might be indeterministic.
> 
> 2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade
> their PostGIS 1.1 installation with it? Would that work?
> Having a lower bound for a matching version might be a good idea,
> although I have no idea how to do that.

Following that reasoning, couldn't a rogue actor inject a fake file (perhaps
bundled with another innocent looking extension) which takes precedence in
wildcard matching?

--
Daniel Gustafsson   https://vmware.com/

Re: Improving connection scalability (src/backend/storage/ipc/procarray.c)

[Daniel Gustafsson]

> On 28 May 2022, at 16:12, Ranier Vilela  wrote:

> Just because I don't have enough hardware to force GetSnapShotData() doesn't 
> mean optimizing it won't make a difference. 

Quoting Andres from upthread:

"To improve something like GetSnapshotData() you first have to come up with
a workload that shows it being a meaningful part of a profile.  Unless it
is, performance differences are going to just be due to various forms of
noise."

If you think this is a worthwhile improvement, you need to figure out a way to
reliably test it in order to prove it.

--
Daniel Gustafsson   https://vmware.com/

Re: postgres and initdb not working inside docker

[Daniel Gustafsson]

> On 28 May 2022, at 14:59, Roffild  wrote:

> This patch fixes an issue inside Docker and will not affect other builds.

Looks like you generated the patch backwards, it's removing the lines you
propose to add.

--
Daniel Gustafsson   https://vmware.com/

Ignoring BRIN for HOT udpates seems broken

[Tomas Vondra]

Hi,

while working on some BRIN stuff, I realized (my) commit 5753d4ee320b
ignoring BRIN indexes for HOT is likely broken. Consider this example:

--
CREATE TABLE t (a INT) WITH (fillfactor = 10);

INSERT INTO t SELECT i
  FROM generate_series(0,10) s(i);

CREATE INDEX ON t USING BRIN (a);

UPDATE t SET a = 0 WHERE random() < 0.01;

SET enable_seqscan = off;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;

SET enable_seqscan = on;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
--

which unfortunately produces this:

  QUERY PLAN
---
 Bitmap Heap Scan on t (actual rows=23 loops=1)
   Recheck Cond: (a = 0)
   Rows Removed by Index Recheck: 2793
   Heap Blocks: lossy=128
   ->  Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1)
 Index Cond: (a = 0)
 Planning Time: 0.049 ms
 Execution Time: 0.424 ms
(8 rows)

SET
   QUERY PLAN
-
 Seq Scan on t (actual rows=995 loops=1)
   Filter: (a = 0)
   Rows Removed by Filter: 99006
 Planning Time: 0.027 ms
 Execution Time: 7.670 ms
(5 rows)

That is, the index fails to return some of the rows :-(

I don't remember the exact reasoning behind the commit, but the commit
message justifies the change like this:

There are no index pointers to individual tuples in BRIN, and the
page range summary will be updated anyway as it relies on visibility
info.

AFAICS that's a misunderstanding of how BRIN uses visibility map, or
rather does not use. In particular, bringetbitmap() does not look at the
vm at all, so it'll produce incomplete bitmap.

So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a
live issue. I don't quite see if this can be salvaged - I'll think about
this a bit more, but it'll probably end with a revert.


regards

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

brin.sql
Description: application/sql

Re: [PATCH] Support % wildcard in extension upgrade filenames

[Laurenz Albe]

On Fri, 2022-05-27 at 17:37 -0400, Regina Obe wrote:
> > At PostGIS we've been thinking of ways to have better support, from
> > PostgreSQL proper, to our upgrade strategy, which has always consisted in a
> > SINGLE upgrade file good for upgrading from any older version.
> > 
> > The current lack of such support for EXTENSIONs forced us to install a lot 
> > of
> > files on disk and we'd like to stop doing that at some point in the future.
> > 
> > The proposal is to support wildcards for versions encoded in the filename so
> > that (for our case) a single wildcard could match "any version". I've been
> > thinking about the '%' character for that, to resemble the wildcard used for
> > LIKE.
> > 
> > Here's the proposal:
> > https://lists.osgeo.org/pipermail/postgis-devel/2022-February/029500.html
> > 
> > A very very short (and untested) patch which might (or might not) support 
> > our
> > case is attached.
> > 
> > The only problem with my proposal/patch would be the presence, on the wild,
> > of PostgreSQL EXTENSION actually using the '%' character in their version
> > strings, which is currently considered legit by PostgreSQL.
> > 
> > How do you feel about the proposal (which is wider than the patch) ?
> 
> Just a heads up about the above, Sandro has added it as a commitfest item
> which hopefully we can polish in time for PG16.
> https://commitfest.postgresql.org/38/3654/
> 
> Does anyone think this is such a horrible idea that we should abandon all
> hope?
> 
> The main impetus is that many extensions (postgis, pgRouting, and I'm sure
> others) have over 300 extensions script files that are exactly the same.
> We'd like to reduce this footprint significantly.
> 
> strk said the patch is crappy so don't look at it just yet.  We'll work on
> polishing it.  I'll review and provide docs for it.

I don't think this idea is fundamentally wrong, but I have two worries:

1. It would be a good idea good to make sure that there is not both
   "extension--%--2.0.sql" and "extension--1.0--2.0.sql" present.
   Otherwise the behavior might be indeterministic.

2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade
   their PostGIS 1.1 installation with it?  Would that work?
   Having a lower bound for a matching version might be a good idea,
   although I have no idea how to do that.

Yours,
Laurenz Albe

Re: Improving connection scalability (src/backend/storage/ipc/procarray.c)

[Ranier Vilela]

Em sáb., 28 de mai. de 2022 às 09:00, Tomas Vondra <
tomas.von...@enterprisedb.com> escreveu:

> On 5/28/22 02:15, Ranier Vilela wrote:
> >
> >
> > Em sex., 27 de mai. de 2022 às 18:08, Andres Freund  > > escreveu:
> >
> > Hi,
> >
> > On 2022-05-27 03:30:46 +0200, Tomas Vondra wrote:
> > > On 5/27/22 02:11, Ranier Vilela wrote:
> > > > ./pgbench -M prepared -c $conns -j $conns -T 60 -S -n -U postgres
> > > >
> > > > pgbench (15beta1)
> > > > transaction type: 
> > > > scaling factor: 1
> > > > query mode: prepared
> > > > number of clients: 100
> > > > number of threads: 100
> > > > maximum number of tries: 1
> > > > duration: 60 s
> > > >
> > > > connstps head  tps patched
> > > >
> > > > 1 17126.32610817792.414234
> > > > 10   82068.12338382468.334836
> > > > 50   73808.73140474678.839428
> > > > 80   73290.19171373116.553986
> > > > 90   67558.48304368384.906949
> > > > 100 65960.98280166997.793777
> > > > 200 62216.01199862870.243385
> > > > 300 62924.22565862796.157548
> > > > 400 62278.09970463129.555135
> > > > 500 63257.93087062188.825044
> > > > 600 61479.89061161517.913967
> > > > 700 61139.35405361327.898847
> > > > 800 60833.66379161517.913967
> > > > 900 61305.12964261248.336593
> > > > 1000   60990.91871961041.670996
> > > >
> > >
> > > These results look much saner, but IMHO it also does not show any
> > clear
> > > benefit of the patch. Or are you still claiming there is a benefit?
> >
> > They don't look all that sane to me - isn't that way lower than one
> > would
> > expect?
> >
> > Yes, quite disappointing.
> >
> > Restricting both client and server to the same four cores, a
> > thermically challenged older laptop I have around I get 150k tps at
> > both 10
> > and 100 clients.
> >
> > And you can share the benchmark details? Hardware, postgres and pgbench,
> > please?
> >
> >
> > Either way, I'd not expect to see any GetSnapshotData() scalability
> > effects to
> > show up on an "Intel® Core™ i5-8250U CPU Quad Core" - there's just
> > not enough
> > concurrency.
> >
> > This means that our customers will not see any connections scalability
> > with PG15, using the simplest hardware?
> >
>
> No. It means that on 4-core machine GetSnapshotData() is unlikely to be
> a bottleneck, because you'll hit various other bottlenecks way earlier.
>
> I personally doubt it even makes sense to worry about scaling to this
> many connections on such tiny system too much.
>

> >
> > The correct pieces of these changes seem very unlikely to affect
> > GetSnapshotData() performance meaningfully.
> >
> > To improve something like GetSnapshotData() you first have to come
> > up with a
> > workload that shows it being a meaningful part of a profile. Unless
> > it is,
> > performance differences are going to just be due to various forms of
> > noise.
> >
> > Actually in the profiles I got with perf, GetSnapShotData() didn't show
> up.
> >
>
> But that's exactly the point Andres is trying to make - if you don't see
> GetSnapshotData() in the perf profile, why do you think optimizing it
> will have any meaningful impact on throughput?
>
You see, I've seen in several places that GetSnapShotData() is the
bottleneck in scaling connections.
One of them, if I remember correctly, was at an IBM in Russia.
Another statement occurs in [1][2][3]
Just because I don't have enough hardware to force GetSnapShotData()
doesn't mean optimizing it won't make a difference.
And even on my modest hardware, we've seen gains, small but consistent.
So IMHO everyone will benefit, including the small servers.

regards,
Ranier Vilela

[1]
https://techcommunity.microsoft.com/t5/azure-database-for-postgresql/improving-postgres-connection-scalability-snapshots/ba-p/1806462
[2] https://www.postgresql.org/message-id/5198715A.6070808%40vmware.com
[3]
https://it-events.com/system/attachments/files/000/001/098/original/PostgreSQL_%D0%BC%D0%B0%D1%81%D1%88%D1%82%D0%B0%D0%B1%D0%B8%D1%80%D0%BE%D0%B2%D0%B0%D0%BD%D0%B8%D0%B5.pdf?1448975472


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

Re: docs: mention "pg_read_all_stats" in "track_activities" description

[Nathan Bossart]

On Sat, May 28, 2022 at 05:50:35PM +0900, Michael Paquier wrote:
> On Wed, May 25, 2022 at 01:04:04PM +0900, Michael Paquier wrote:
>> Good point.  So this would give, to be exact:
>> "This information is only visible to superusers, roles with privileges
>> of the pg_read_all_stats role, and and the user owning the sessionS
>> being reported on (including sessions belonging to a role they have
>> the privileges of)."
> 
> Nathan, Ian, if you think that this could be worded better, please
> feel free to let me know.  Thanks.

Sorry, I missed this one earlier.  I'm okay with something along those
lines.  I'm still trying to think of ways to make the last part a little
clearer, but I don't have any ideas beyond what we've discussed upthread.

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

Re: postgres and initdb not working inside docker

[Roffild]

Add --disable-check-permissions to ./configure

After applying the patch, run "autoheader -f ; autoconf"

This patch fixes an issue inside Docker and will not affect other builds.diff --git a/configure.ac b/configure.ac
index 3f0077696b..d093fb88dd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -749,14 +749,6 @@ PGAC_ARG_BOOL(enable, cassert, no, [enable assertion 
checks (for debugging)],
  [Define to 1 to build with assertion checks. 
(--enable-cassert)])])
 
 
-#
-# Disable file permission checks
-#
-PGAC_ARG_BOOL(enable, check-permissions, yes, [disable file permission checks 
(for Docker)],
-  [AC_DEFINE([ENABLE_CHECK_PERMISSIONS], 1,
- [Define to 1 to build with permission checks. 
(--disable-check-permissions)])])
-
-
 #
 # Include directories
 #
diff --git a/src/backend/utils/init/miscinit.c 
b/src/backend/utils/init/miscinit.c
index bcd56cc7cb..ec6a61594a 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -351,7 +351,7 @@ checkDataDir(void)
 *
 * XXX can we safely enable this check on Windows?
 */
-#if defined(ENABLE_CHECK_PERMISSIONS) && !defined(WIN32) && 
!defined(__CYGWIN__)
+#if !defined(WIN32) && !defined(__CYGWIN__)
if (stat_buf.st_uid != geteuid())
ereport(FATAL,

(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -371,7 +371,7 @@ checkDataDir(void)
 * be proper support for Unix-y file permissions.  Need to think of a
 * reasonable check to apply on Windows.
 */
-#if defined(ENABLE_CHECK_PERMISSIONS) && !defined(WIN32) && 
!defined(__CYGWIN__)
+#if !defined(WIN32) && !defined(__CYGWIN__)
if (stat_buf.st_mode & PG_MODE_MASK_GROUP)
ereport(FATAL,

(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),

Re: Assert name/short_desc to prevent SHOW ALL segfault

[Nathan Bossart]

On Sat, May 28, 2022 at 12:26:34PM +0900, Michael Paquier wrote:
> On Fri, May 27, 2022 at 10:43:17AM -0700, Nathan Bossart wrote:
>> Makes sense.  Here's a new patch set.  0001 is the part intended for
>> back-patching, and 0002 is the rest (i.e., adding pg_attribute_nonnull()).
>> I switched to using __has_attribute to discover whether nonnull was
> 
> Okay, I have looked at 0001 this morning and applied it down to 12.
> The change in GetConfigOptionByNum() is not required in 10 and 11, as
> the strings of pg_show\all_settings() have begun to be translated in
> 12~.

Thanks!

>> supported, as that seemed cleaner.  I didn't see any need for a new
>> configure check, but maybe I am missing something.
> 
> And I've learnt today that we enforce a definition of __has_attribute
> at the top of c.h, and that we already rely on that.  So I agree that
> what you are doing in 0002 should be enough.  Should we wait until 16~
> opens for business though?  I don't see a strong argument to push
> forward with that now that we are in beta mode on HEAD.

Yeah, I see no reason that this should go into v15.  I created a new
commitfest entry so that this isn't forgotten:

https://commitfest.postgresql.org/38/3655/

And I've reposted 0002 here so that we get some cfbot coverage in the
meantime.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 88ac1a95e84998ce473d461424b81b91a0a3e3cc Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 27 May 2022 10:10:09 -0700
Subject: [PATCH v5 1/1] Introduce pg_attribute_nonnull() and use it for
 DefineCustomXXXVariable().

pg_attribute_nonnull() can be used to generate compiler warnings
when a function is called with the specified arguments set to NULL.
An empty argument list indicates that all pointer arguments must
not be NULL.  pg_attribute_nonnull() only works for compilers that
support the nonnull function attribute.  If nonnull is not
supported, pg_attribute_nonnull() has no effect.

In addition to introducing pg_attribute_nonnull(), this change uses
it for the DefineCustomXXXVariable() functions to generate warnings
when the "name" and "valueAddr" arguments are set to NULL.

Idea by: Andres Freund
Author: Nathan Bossart
Reviewed by: Michael Paquier, Tom Lane
Discussion: https://postgr.es/m/20220525061739.ur7x535vtzyzkmqo%40alap3.anarazel.de
---
 src/include/c.h | 11 +++
 src/include/utils/guc.h | 10 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/include/c.h b/src/include/c.h
index 4f16e589b3..0c64557c62 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -144,6 +144,17 @@
 #define pg_attribute_no_sanitize_alignment()
 #endif
 
+/*
+ * pg_attribute_nonnull means the compiler should warn if the function is called
+ * with the listed arguments set to NULL.  If no arguments are listed, the
+ * compiler should warn if any pointer arguments are set to NULL.
+ */
+#if __has_attribute (nonnull)
+#define pg_attribute_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
+#else
+#define pg_attribute_nonnull(...)
+#endif
+
 /*
  * Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
  * used in assert-enabled builds, to avoid compiler warnings about unused
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..be691c5e9c 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -303,7 +303,7 @@ extern void DefineCustomBoolVariable(const char *name,
 	 int flags,
 	 GucBoolCheckHook check_hook,
 	 GucBoolAssignHook assign_hook,
-	 GucShowHook show_hook);
+	 GucShowHook show_hook) pg_attribute_nonnull(1, 4);
 
 extern void DefineCustomIntVariable(const char *name,
 	const char *short_desc,
@@ -316,7 +316,7 @@ extern void DefineCustomIntVariable(const char *name,
 	int flags,
 	GucIntCheckHook check_hook,
 	GucIntAssignHook assign_hook,
-	GucShowHook show_hook);
+	GucShowHook show_hook) pg_attribute_nonnull(1, 4);
 
 extern void DefineCustomRealVariable(const char *name,
 	 const char *short_desc,
@@ -329,7 +329,7 @@ extern void DefineCustomRealVariable(const char *name,
 	 int flags,
 	 GucRealCheckHook check_hook,
 	 GucRealAssignHook assign_hook,
-	 GucShowHook show_hook);
+	 GucShowHook show_hook) pg_attribute_nonnull(1, 4);
 
 extern void DefineCustomStringVariable(const char *name,
 	   const char *short_desc,
@@ -340,7 +340,7 @@ extern void DefineCustomStringVariable(const char *name,
 	   int flags,
 	   GucStringCheckHook check_hook,
 	   GucStringAssignHook assign_hook,
-	   GucShowHook show_hook);
+	   GucShowHook show_hook) pg_attribute_nonnull(1, 4);
 
 extern void DefineCustomEnumVariable(const char *name,
 	 const char *short_desc,
@@ -352,7 +352,7 @@ extern void DefineCustomEnumVariable(const char *name,
 	 int flags,
 	 

Re: Improving connection scalability (src/backend/storage/ipc/procarray.c)

[Tomas Vondra]

On 5/28/22 02:36, Ranier Vilela wrote:
> Em sex., 27 de mai. de 2022 às 18:22, Andres Freund  > escreveu:
> 
> Hi,
> 
> On 2022-05-27 10:35:08 -0300, Ranier Vilela wrote:
> > Em qui., 26 de mai. de 2022 às 22:30, Tomas Vondra <
> > tomas.von...@enterprisedb.com
> > escreveu:
> >
> > > On 5/27/22 02:11, Ranier Vilela wrote:
> > > >
> > > > ...
> > > >
> > > > Here the results with -T 60:
> > >
> > > Might be a good idea to share your analysis / interpretation of the
> > > results, not just the raw data. After all, the change is being
> proposed
> > > by you, so do you think this shows the change is beneficial?
> > >
> > I think so, but the expectation has diminished.
> > I expected that the more connections, the better the performance.
> > And for both patch and head, this doesn't happen in tests.
> > Performance degrades with a greater number of connections.
> 
> Your system has four CPUs. Once they're all busy, adding more
> connections
> won't improve performance. It'll just add more and more context
> switching,
> cache misses, and make the OS scheduler do more work.
> 
> conns       tps head
> 10      82365.634750
> 50      74593.714180
> 80      69219.756038
> 90      67419.574189
> 100     66613.771701
> Yes it is quite disappointing that with 100 connections, tps loses to 10
> connections.
>  

IMO that's entirely expected on a system with only 4 cores. Increasing
the number of connections inevitably means more overhead (you have to
track/manage more stuff). And at some point the backends start competing
for L2/L3 caches, context switches are not free either, etc. So once you
cross ~2-3x the number of cores, you should expect this.

This behavior is natural/inherent, it's unlikely to go away, and it's
one of the reasons why we recommend not to use too many connections. If
you try to maximize throughput, just don't do that. Or just use machine
with more cores.

> 
> > GetSnapShowData() isn't a bottleneck?
> 
> I'd be surprised if it showed up in a profile on your machine with that
> workload in any sort of meaningful way. The snapshot reuse logic
> will always
> work - because there are no writes - and thus the only work that
> needs to be
> done is to acquire the ProcArrayLock briefly.  And because there is
> only a
> small number of cores, contention on the cacheline for that isn't a
> problem.
> 
> Thanks for sharing this.
>  
> 
> 
> 
> > > These results look much saner, but IMHO it also does not show
> any clear
> > > benefit of the patch. Or are you still claiming there is a benefit?
> > >
> > We agree that they are micro-optimizations.  However, I think they
> should be
> > considered micro-optimizations in inner loops, because all in
> procarray.c is
> > a hotpath.
> 
> As explained earlier, I don't agree that they optimize anything - you're
> making some of the scalability behaviour *worse*, if it's changed at
> all.
> 
> 
> > The first objective, I believe, was achieved, with no performance
> > regression.
> > I agree, the gains are small, by the tests done.
> 
> There are no gains.
> 
> IMHO, I must disagree.
>  

You don't have to, really. What you should do is showing results
demonstrating the claimed gains, and so far you have not done that.

I don't want to be rude, but so far you've shown results from a
benchmark testing fork(), due to only running 10 transactions per
client, and then results from a single run for each client count (which
doesn't really show any gains either, and is so noisy).

As mentioned GetSnapshotData() is not even in perf profile, so why would
the patch even make a difference?

You've also claimed it helps generating better code on older compilers,
but you've never supported that with any evidence.


Maybe there is an improvement - show us. Do a benchmark with more runs,
to average-out the noise. Calculate VAR/STDEV to show how variable the
results are. Use that to compare results and decide if there is an
improvement. Also, keep in mind binary layout matters [1].

[1] https://www.youtube.com/watch?v=r-TLSBdHe1A

> 
> 
> 
> > But, IMHO, this is a good way, small gains turn into big gains in
> the end,
> > when applied to all code.
> >
> > Consider GetSnapShotData()
> > 1. Most of the time the snapshot is not null, so:
> > if (snaphost == NULL), will fail most of the time.
> >
> > With the patch:
> > if (snapshot->xip != NULL)
> > {
> >     if (GetSnapshotDataReuse(snapshot))
> >      return snapshot;
> > }
> >
> > Most of the time the test is true and GetSnapshotDataReuse is not
> called
> > for new
> > snapshots.
> > count, subcount and  suboverflowed, will not be initialized, for all
> > snapshots.
> 
> 

Re: Improving connection scalability (src/backend/storage/ipc/procarray.c)

[Tomas Vondra]

On 5/28/22 02:15, Ranier Vilela wrote:
> 
> 
> Em sex., 27 de mai. de 2022 às 18:08, Andres Freund  > escreveu:
> 
> Hi,
> 
> On 2022-05-27 03:30:46 +0200, Tomas Vondra wrote:
> > On 5/27/22 02:11, Ranier Vilela wrote:
> > > ./pgbench -M prepared -c $conns -j $conns -T 60 -S -n -U postgres
> > >
> > > pgbench (15beta1)
> > > transaction type: 
> > > scaling factor: 1
> > > query mode: prepared
> > > number of clients: 100
> > > number of threads: 100
> > > maximum number of tries: 1
> > > duration: 60 s
> > >
> > > conns    tps head      tps patched
> > >
> > > 1         17126.326108            17792.414234
> > > 10       82068.123383            82468.334836
> > > 50       73808.731404            74678.839428
> > > 80       73290.191713            73116.553986
> > > 90       67558.483043            68384.906949
> > > 100     65960.982801            66997.793777
> > > 200     62216.011998            62870.243385
> > > 300     62924.225658            62796.157548
> > > 400     62278.099704            63129.555135
> > > 500     63257.930870            62188.825044
> > > 600     61479.890611            61517.913967
> > > 700     61139.354053            61327.898847
> > > 800     60833.663791            61517.913967
> > > 900     61305.129642            61248.336593
> > > 1000   60990.918719            61041.670996
> > >
> >
> > These results look much saner, but IMHO it also does not show any
> clear
> > benefit of the patch. Or are you still claiming there is a benefit?
> 
> They don't look all that sane to me - isn't that way lower than one
> would
> expect?
> 
> Yes, quite disappointing.
> 
> Restricting both client and server to the same four cores, a
> thermically challenged older laptop I have around I get 150k tps at
> both 10
> and 100 clients.
> 
> And you can share the benchmark details? Hardware, postgres and pgbench,
> please?
> 
> 
> Either way, I'd not expect to see any GetSnapshotData() scalability
> effects to
> show up on an "Intel® Core™ i5-8250U CPU Quad Core" - there's just
> not enough
> concurrency.
> 
> This means that our customers will not see any connections scalability
> with PG15, using the simplest hardware?
> 

No. It means that on 4-core machine GetSnapshotData() is unlikely to be
a bottleneck, because you'll hit various other bottlenecks way earlier.

I personally doubt it even makes sense to worry about scaling to this
many connections on such tiny system too much.

> 
> The correct pieces of these changes seem very unlikely to affect
> GetSnapshotData() performance meaningfully.
> 
> To improve something like GetSnapshotData() you first have to come
> up with a
> workload that shows it being a meaningful part of a profile. Unless
> it is,
> performance differences are going to just be due to various forms of
> noise.
> 
> Actually in the profiles I got with perf, GetSnapShotData() didn't show up.
> 

But that's exactly the point Andres is trying to make - if you don't see
GetSnapshotData() in the perf profile, why do you think optimizing it
will have any meaningful impact on throughput?


regards

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

Re: Ignore heap rewrites for materialized views in logical replication

[Amit Kapila]

On Sat, May 28, 2022 at 2:44 AM Euler Taveira  wrote:
>
> While investigating an internal report, I concluded that it is a bug. The
> reproducible test case is simple (check 0002) and it consists of a FOR ALL
> TABLES publication and a non-empty materialized view on publisher. After the
> setup, if you refresh the MV, you got the following message on the subscriber:
>
> ERROR:  logical replication target relation "public.pg_temp_N" does not 
> exist
>
> That's because the commit 1a499c2520 (that fixes the heap rewrite for tables)
> forgot to consider that materialized views can also create transient heaps and
> they should also be skipped. The affected version is only 10 because 11
> contains a different solution (commit 325f2ec555) that provides a proper fix
> for the heap rewrite handling in logical decoding.
>
> 0001 is a patch to skip MV too.
>

I agree with your analysis and the fix looks correct to me.

> I attached 0002 to demonstrate the issue but it
> doesn't seem appropriate to be included. The test was written to detect the
> error and bail out. After this fix, it takes a considerable amount of time to
> finish the test because it waits for a message that never arrives.
>

Instead of waiting for an error, we can try to insert into a new table
created by the test case after the 'Refresh ..' command and wait for
the change to be replicated by using wait_for_caught_up.

> Since nobody
> reports this bug in 5 years and considering that version 10 will be EOL in 6
> months, I don't think an additional test is crucial here.
>

Let's try to see if we can simplify the test so that it can be
committed along with a fix. If we are not able to find any reasonable
way then we can think of skipping it.

-- 
With Regards,
Amit Kapila.

Re: Prevent writes on large objects in read-only transactions

[Michael Paquier]

On Fri, May 27, 2022 at 03:30:28PM +0900, Yugo NAGATA wrote:
> Currently, lo_creat(e), lo_import, lo_unlink, lowrite, lo_put,
> and lo_from_bytea are allowed even in read-only transactions.
> By using them, pg_largeobject and pg_largeobject_metatable can
> be modified in read-only transactions and the effect remains
> after the transaction finished. Is it unacceptable behaviours, 
> isn't it?

Well, there is an actual risk to break applications that have worked
until now for a behavior that has existed for years with zero
complaints on the matter, so I would leave that alone.  Saying that, I
don't really disagree with improving the error messages a bit if we
are in recovery.
--
Michael


signature.asc
Description: PGP signature

Re: pg_upgrade test writes to source directory

[Michael Paquier]

On Fri, May 27, 2022 at 02:45:57PM +0200, Peter Eisentraut wrote:
> I think you can just chdir to ${PostgreSQL::Test::Utils::tmp_check}.

Hmm.  I think that I prefer your initial suggestion with TESTOUTDIR.
This sticks better in the long term, while making things consistent
with 010_tab_completion.pl, the only test that moves to TESTDIR while
running.  So my vote would be to backpatch first the addition of
TESTOUTDIR, then fix the TAP test of pg_upgrade on HEAD to do the
same.

And I have just noticed that I completely forgot to add Andres about
this specific point, as meson is his work.  So done now.
--
Michael


signature.asc
Description: PGP signature

Re: docs: mention "pg_read_all_stats" in "track_activities" description

[Michael Paquier]

On Wed, May 25, 2022 at 01:04:04PM +0900, Michael Paquier wrote:
> Good point.  So this would give, to be exact:
> "This information is only visible to superusers, roles with privileges
> of the pg_read_all_stats role, and and the user owning the sessionS
> being reported on (including sessions belonging to a role they have
> the privileges of)."

Nathan, Ian, if you think that this could be worded better, please
feel free to let me know.  Thanks.
--
Michael


signature.asc
Description: PGP signature

Re: suboverflowed subtransactions concurrency performance optimize

[Michael Paquier]

On Fri, May 27, 2022 at 08:55:02AM -0700, Andres Freund wrote:
> On 2022-05-27 15:44:39 +0530, Amit Kapila wrote:
>> Won't in theory the similar cache in transam.c is also prone to
>> similar behavior?

TransactionIdDidCommit() and TransactionIdDidAbort() are used in much
more code paths for visibility purposes, contrary to the subtrans.c
ones.

> It's not quite the same risk, because there we are likely to actually hit the
> cache regularly. Whereas quite normal workloads might not hit this cache for
> days on end.

Yeah.  In short, this mostly depends on the use of savepoints and the
number of XIDs issued until PGPROC_MAX_CACHED_SUBXIDS is reached, and
a single cache entry in this code path would reduce the pressure on
the SLRU lookups depending on the number of queries issued, for
example.  One thing I know of that likes to abuse of savepoints and
could cause overflows to make this easier to hit is the ODBC driver
coupled with short queries in long transactions, where its internals
enforce the use of a savepoint each time a query is issued by an
application (pretty much what the benchmark at the top of the thread
does).  In this case, even the single cache approach would not help
much because I recall that we finish with one savepoint per query to
be able to rollback to any previous state within a given transaction
(as the ODBC APIs allow).

Doing a caching within SubTransGetParent() would be more interesting,
for sure, though the invalidation to clean the cache and to make that
robust enough may prove tricky.

It took me some time to come back to this thread.  The change has now
been reverted.
--
Michael


signature.asc
Description: PGP signature