Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error

[Julien Rouhaud]

Hi,

On Sat, Sep 03, 2022 at 10:36:37AM +0500, Ibrar Ahmed wrote:
>
> Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson,
>
> Since you haven't had time to write a review the last many days, the author
> replied
> with a rebased patch for a long time and never heard. We've taken your name
> off the reviewer list for this patch. Of course, you are still welcome to
> review it if you can
> find the time. We're removing your name so that other reviewers know the
> patch still needs
> attention. We understand that day jobs and other things get in the way of
> doing patch
> reviews when you want to, so please come back and review a patch or two
> later when you
> have more time.

I thought that we decided not to remove assigned reviewers from a CF entry,
even if they didn't reply recently?  See the discussion around
https://www.postgresql.org/message-id/CA%2BTgmoZSBNhX0zCkG5T5KiQize9Aq4%2Bec%2BuqLcfBhm_%2B12MbQA%40mail.gmail.com

Re: Fix typo function circle_same (src/backend/utils/adt/geo_ops.c)

[Julien Rouhaud]

Hi,

On Fri, Sep 01, 2022 at 09:55:15PM +0200, Daniel Gustafsson wrote:
> > On 2 Sep 2022, at 21:22, Ranier Vilela  wrote:
> >
> > Em sex., 2 de set. de 2022 às 16:15, Daniel Gustafsson  > > escreveu:
> > > On 2 Sep 2022, at 21:08, Ranier Vilela  > > > wrote:
> >
> > > At function circle_same the second isnan test is wrong.
> >
> > Yeah, that seems pretty wrong.  Did you attempt to procure a test for when 
> > this
> > yields the wrong result?
> > Hi Daniel,
> > Unfortunately not.
>
> On HEAD, the below query yields what seems to be the wrong result for the 
> "same
> as" operator:
>
> postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
>  ?column?
> --
>  t
> (1 row)
>
> With the patch applied, it returns the expected:
>
> postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
>  ?column?
> --
>  f
> (1 row)
>
> There seems to be surprisingly few tests around these geo operators?

Yeah, there are unfortunately a lot of problems around those and NaN, with
multiple reports in the past (I recall [1] and [2] but there were others).
There was a CF entry that tried to improve things [3], part of it was committed
but not all [4], and clearly some more work is needed.

[1] 
https://www.postgresql.org/message-id/flat/cagf+fx70rwfok5cd00umfa__0yp+vtqg5ck7c2onb-yczp0...@mail.gmail.com
[2] https://www.postgresql.org/message-id/20210330095751.x5hnqbqcxilzwjlm@nol
[3] https://commitfest.postgresql.org/38/2710/
[4] https://www.postgresql.org/message-id/3558828.1659045...@sss.pgh.pa.us

Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work

[Michael Paquier]

On Sat, Sep 03, 2022 at 10:47:32AM +0500, Ibrar Ahmed wrote:
> Your patch require a rebase. Please provide the latest rebase patch.

I was looking for a CF entry yesterday when I looked at this patch,
missing https://commitfest.postgresql.org/39/3496/.  This has been
applied as of bfb9dfd.
--
Michael


signature.asc
Description: PGP signature

Re: making relfilenodes 56 bits

[Dilip Kumar]

On Tue, Aug 30, 2022 at 9:23 PM Robert Haas  wrote:

> Well, that's very awkward. It doesn't seem like it would be very
> difficult to teach pg_upgrade to call pg_restore without --clean and
> just do the drop database itself, but that doesn't really help,
> because pg_restore will in any event be creating the new database.
> That doesn't seem like something we can practically refactor out,
> because only pg_dump knows what properties to use when creating the
> new database. What we could do is have the dump include a command like
> SELECT pg_binary_upgrade_move_things_out_of_the_way(some_arguments_here),
> but that doesn't really help very much, because passing the whole list
> of relfilenode values from the old database seems pretty certain to be
> a bad idea. The whole idea here was that we'd be able to build a hash
> table on the new database's system table OIDs, and it seems like
> that's not going to work.

Right.

> We could try to salvage some portion of the idea by making
> pg_binary_upgrade_move_things_out_of_the_way() take a more restricted
> set of arguments, like the smallest and largest relfilenode values
> from the old database, and then we'd just need to move things that
> overlap. But that feels pretty hit-or-miss to me as to whether it
> actually avoids any work, and
> pg_binary_upgrade_move_things_out_of_the_way() might also be annoying
> to write. So perhaps we have to go back to the drawing board here.

So as of now, we have two open options 1) the current approach and
what patch is following to use Oid as relfilenode for the system
tables when initially created.  2) call
pg_binary_upgrade_move_things_out_of_the_way() which force rewrite all
the system tables.

Another idea that I am not very sure how feasible is. Can we change
the dump such that in binary upgrade mode it will not use template0 as
a template database (in creating database command) but instead some
new database as a template e.g. template-XYZ?   And later for conflict
checking, we will create this template-XYZ database on the new cluster
and then we will perform all the conflict check (from all the
databases of the old cluster) and rewrite operations on this database.
And later all the databases will be created using template-XYZ as the
template and all the rewriting stuff we have done is still intact.
The problems I could think of are 1) only for a binary upgrade we will
have to change the pg_dump.  2) we will have to use another database
name as the reserved database name but what if that name is already in
use in the previous cluster?

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

vacuumlo: add test to vacuumlo for test coverage

[Dong Wook Lee]

Hi hackers,
I write a tiny patch about vacuumlo to improve test coverage.
I hope my work is meaningful.

---
Regards,
DongWook Lee.


v1_add_test_to_vacuumlo.patch
Description: Binary data

Re: making relfilenodes 56 bits

[Dilip Kumar]

On Sat, Sep 3, 2022 at 1:50 PM Dilip Kumar  wrote:
>
> On Tue, Aug 30, 2022 at 9:23 PM Robert Haas  wrote:
>
> > Well, that's very awkward. It doesn't seem like it would be very
> > difficult to teach pg_upgrade to call pg_restore without --clean and
> > just do the drop database itself, but that doesn't really help,
> > because pg_restore will in any event be creating the new database.
> > That doesn't seem like something we can practically refactor out,
> > because only pg_dump knows what properties to use when creating the
> > new database. What we could do is have the dump include a command like
> > SELECT pg_binary_upgrade_move_things_out_of_the_way(some_arguments_here),
> > but that doesn't really help very much, because passing the whole list
> > of relfilenode values from the old database seems pretty certain to be
> > a bad idea. The whole idea here was that we'd be able to build a hash
> > table on the new database's system table OIDs, and it seems like
> > that's not going to work.
>
> Right.
>
> > We could try to salvage some portion of the idea by making
> > pg_binary_upgrade_move_things_out_of_the_way() take a more restricted
> > set of arguments, like the smallest and largest relfilenode values
> > from the old database, and then we'd just need to move things that
> > overlap. But that feels pretty hit-or-miss to me as to whether it
> > actually avoids any work, and
> > pg_binary_upgrade_move_things_out_of_the_way() might also be annoying
> > to write. So perhaps we have to go back to the drawing board here.
>
> So as of now, we have two open options 1) the current approach and
> what patch is following to use Oid as relfilenode for the system
> tables when initially created.  2) call
> pg_binary_upgrade_move_things_out_of_the_way() which force rewrite all
> the system tables.
>
> Another idea that I am not very sure how feasible is. Can we change
> the dump such that in binary upgrade mode it will not use template0 as
> a template database (in creating database command) but instead some
> new database as a template e.g. template-XYZ?   And later for conflict
> checking, we will create this template-XYZ database on the new cluster
> and then we will perform all the conflict check (from all the
> databases of the old cluster) and rewrite operations on this database.
> And later all the databases will be created using template-XYZ as the
> template and all the rewriting stuff we have done is still intact.
> The problems I could think of are 1) only for a binary upgrade we will
> have to change the pg_dump.  2) we will have to use another database
> name as the reserved database name but what if that name is already in
> use in the previous cluster?

While we are still thinking on this issue, I have rebased the patch on
the latest head and fixed a couple of minor issues.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From a645d39c6109269bb74ed8f0627e61ecf9b0535a Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Fri, 26 Aug 2022 10:20:18 +0530
Subject: [PATCH v16] Widen relfilenumber from 32 bits to 56 bits
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently relfilenumber is 32 bits wide and that has a risk of wraparound so
the relfilenumber can be reused.  And to guard against the relfilenumber reuse
there is some complicated hack which leaves a 0-length tombstone file around
until the next checkpoint.  And when we allocate a new relfilenumber
we also need to loop to check the on disk conflict.

As part of this patch we are making the relfilenumber 56 bits wide and there will be
no provision for wraparound.  So after this change we will be able to get rid of the
0-length tombstone file and the loop for checking the on-disk conflict of the
relfilenumbers.

The reason behind making it 56 bits wide instead of directly making 64 bits wide is
that if we make it 64 bits wide then the size of the BufferTag will be increased which
will increase the memory usage and that may also impact the performance.  So in order
to avoid that, inside the buffer tag, we will use 8 bits for the fork number and 56 bits
for the relfilenumber.
---
 contrib/pg_buffercache/Makefile|   4 +-
 .../pg_buffercache/pg_buffercache--1.3--1.4.sql|  30 
 contrib/pg_buffercache/pg_buffercache.control  |   2 +-
 contrib/pg_buffercache/pg_buffercache_pages.c  |  39 -
 contrib/pg_prewarm/autoprewarm.c   |   4 +-
 contrib/pg_walinspect/expected/pg_walinspect.out   |   4 +-
 contrib/pg_walinspect/sql/pg_walinspect.sql|   4 +-
 contrib/test_decoding/expected/rewrite.out |   2 +-
 contrib/test_decoding/sql/rewrite.sql  |   2 +-
 doc/src/sgml/catalogs.sgml |   2 +-
 doc/src/sgml/pgbuffercache.sgml|   2 +-
 doc/src/sgml/storage.sgml  |   5 +-
 src/backend/access/gin/ginxlog.c   |   2 +-
 src/backend/access/rm

add test to psql for coverage with --help, \e option, encoding option

[Dong Wook Lee]

Hi hackers,
I try to add to psql test about --help, \e, and the encoding option.

---
Regards,
DongWook Lee.

Re: add test to psql for coverage with --help, \e option, encoding option

[Dong Wook Lee]

I confirmed that I missed the patch file.
And the current code is different from when I wrote the patch, so I
don't think my patch will be meaningful anymore.

On Sat, Sep 3, 2022 at 6:08 PM Dong Wook Lee  wrote:
>
> Hi hackers,
> I try to add to psql test about --help, \e, and the encoding option.
>
> ---
> Regards,
> DongWook Lee.


v1_add_test_to_psql.patch
Description: Binary data

Re: making relfilenodes 56 bits

[Amit Kapila]

On Tue, Aug 30, 2022 at 6:15 PM Dilip Kumar  wrote:
>
> On Fri, Aug 26, 2022 at 9:33 PM Robert Haas  wrote:
> >
> > On Fri, Aug 26, 2022 at 7:01 AM Dilip Kumar  wrote:
> > > While working on this solution I noticed one issue. Basically, the
> > > problem is that during binary upgrade when we try to rewrite a heap we
> > > would expect that “binary_upgrade_next_heap_pg_class_oid” and
> > > “binary_upgrade_next_heap_pg_class_relfilenumber” are already set for
> > > creating a new heap. But we are not preserving anything so we don't
> > > have those values. One option to this problem is that we can first
> > > start the postmaster in non-binary upgrade mode perform all conflict
> > > checking and rewrite and stop the postmaster.  Then start postmaster
> > > again and perform the restore as we are doing now.  Although we will
> > > have to start/stop the postmaster one extra time we have a solution.
> >
> > Yeah, that seems OK. Or we could add a new function, like
> > binary_upgrade_allow_relation_oid_and_relfilenode_assignment(bool).
> > Not sure which way is better.
>
> I have found one more issue with this approach of rewriting the
> conflicting table.  Earlier I thought we could do the conflict
> checking and rewriting inside create_new_objects() right before the
> restore command.  But after implementing (while testing) this I
> realized that we DROP and CREATE the database while restoring the dump
> that means it will again generate the conflicting system tables.  So
> theoretically the rewriting should go in between the CREATE DATABASE
> and restoring the object but as of now both create database and
> restoring other objects are part of a single dump file.  I haven't yet
> analyzed how feasible it is to generate the dump in two parts, first
> part just to create the database and in second part restore the rest
> of the object.
>

Isn't this happening because we are passing "--clean
--create"/"--create" options to pg_restore in create_new_objects()? If
so, then I think one idea to decouple would be to not use those
options. Perform drop/create separately via commands (for create, we
need to generate the command as we are generating while generating the
dump in custom format), then rewrite the conflicting tables, and
finally restore the dump.

-- 
With Regards,
Amit Kapila.

Re: win_flex.exe (and likely win_bison.exe) isn't concurrency safe

[Andrew Dunstan]

On 2022-09-02 Fr 18:43, Andres Freund wrote:
> Hi,
>
> building PG with meson on windows I occasionally got weird errors around
> scanners. Sometimes scanner generation would fail with
>
>   win_flex.exe: error deleting file 
> C:\Users\myadmin\AppData\Local\Temp\~flex_out_main_2
>
> sometimes the generated scanner would just be corrupted.
>
>
> I was confused by only hitting this in local VM, not in CI, but after finally
> hunting it down it made more sense:
> https://github.com/lexxmark/winflexbison/issues/86
>
> https://github.com/lexxmark/winflexbison/blob/master/flex/src/main.c#L1051
>
> It uses a temporary file name without any concurrency protection. Looks like
> windows' _tempnam is pretty darn awful and returns a predictable name as long
> as no conflicting file exists.
>
> Our documentation doesn't point to winflexbison, but recommends using
> flex/bison from msys. But I've certainly read about others using winflexbison,
> e.g. [1] [2]. The flex/bison in 'chocolatey', which I've also seen referenced,
> is afaics winflexbison.
>
>
> Afaict the issue also exists in our traditional windows build - but I've not
> seen anybody report this as an issue.
>
>
> I started this thread to document the issue, in case developers using visual
> studio are hitting this today.
>
>
> It looks like a similar issue exists for the windows bison port:
> https://github.com/lexxmark/winflexbison/blob/b2a94ad5fd82cf4a54690a879e14ff9511b77273/bison/src/output.c#L816
>
> I've not observed that failure, presumably because the window for it is much
> shorter.
>
>
> For the meson build it is trivial to "address" this by setting FLEX_TMP_DIR to
> a private directory (which we already need to deal with lex.backup), something
> similar could be done for src/tools/msvc.
>
>
> Unless we think it'd be better to just refuse to work with winflexbison?


No, I think your workaround is better if it works. I don't want to
require installation of msys to build with MSVC if that's avoidable.


cheers


andrew


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

Re: json docs fix jsonb_path_exists_tz again

[Andrew Dunstan]

On 2022-09-02 Fr 20:59, Michael Paquier wrote:
> On Fri, Sep 02, 2022 at 04:25:38PM +0200, Erik Rijkers wrote:
>> In funcs.sgml, the value fed into jsonb_path_exists_tz was wrong; fixed as
>> attached.
>>
>> (was inadvertently reverted with the big JSON revert)
> Yeah, good catch.  This comes from 2f2b18b.  There is a second
> inconsistency with jsonb_set_lax().  I'll go fix both.


Thanks for fixing, you beat me to it.


cheers


andrew

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

Re: Schema variables - new implementation for Postgres 15

[Julien Rouhaud]

Hi,

On Fri, Sep 02, 2022 at 07:42:00AM +0200, Pavel Stehule wrote:
>
> rebased today

After some off-list discussion with Pavel, I'm sending some proposal patches
(in .txt extension) to apply to the last patchset.

To sum up, when a session issues a DROP VARIABLE, the session will receive an
sinval notification for its own drop and we don't want to process it
immediately as we need to hold the value in case the transaction is rollbacked.
The current patch avoided that by forcing a single processing of sinval per
transaction, and forcing it before dropping the variable.  It works but it
seems to me that postponing all but the first VARIABLEOID sinval to the next
transaction is a big hammer, and the sooner we can free some memory the better.

For an alternative approach the attached patch store the lxid in the SVariable
itself when dropping a currently set variable, so we can process all sinval and
simply defer to the next transaction the memory cleanup of the variable(s) we
know we just dropped.  What do you think of that approach?

As I was working on some changes I also made a pass on session_variable.c.  I
tried to improve a bit some comments, and also got rid of the "first_time"
variable.  The name wasn't really great, and AFAICS it can be replaced by
testing whether the memory context has been created yet or not.

But once that done I noticed the get_rowtype_value() function.  I don't think
this function is necessary as the core code already knows how to deal with
stored datum when the underlying composite type was modified.  I tried to
bypass that function and always simply return the stored value and all the
tests run fine.  Is there really any cases when this extra code is needed?

FTR I tried to do a bunch of additional testing using relation as base type for
variable, as you can do more with those than plain composite types, but it
still always works just fine.

However, while doing so I noticed that find_composite_type_dependencies()
failed to properly handle dependencies on relation (plain tables, matviews and
partitioned tables).  I'm also adding 2 additional patches to fix this corner
case and add an additional regression test for the plain table case.
>From 43adcd5ed555fd4920061f643c8392c54d8b625e Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Sat, 3 Sep 2022 16:11:03 +0800
Subject: [PATCH 1/3] FIXUP 0001-catalog-support-for-session-variables

---
 src/backend/commands/tablecmds.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d7418e088e..65e1c1e670 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6518,6 +6518,22 @@ find_composite_type_dependencies(Oid typeOid, Relation 
origRelation,

RelationGetRelationName(origRelation),

get_namespace_name(get_session_variable_namespace(varid)),

get_session_variable_name(varid;
+   else if (origRelation->rd_rel->relkind == 
RELKIND_FOREIGN_TABLE)
+   ereport(ERROR,
+   
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot alter foreign 
table \"%s\" because session variable \"%s.%s\" uses it",
+   
RelationGetRelationName(origRelation),
+   
get_namespace_name(get_session_variable_namespace(varid)),
+   
get_session_variable_name(varid;
+   else if (origRelation->rd_rel->relkind == 
RELKIND_RELATION ||
+   origRelation->rd_rel->relkind == 
RELKIND_MATVIEW ||
+   origRelation->rd_rel->relkind == 
RELKIND_PARTITIONED_TABLE)
+   ereport(ERROR,
+   
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot alter table 
\"%s\" because session variable \"%s.%s\" uses it",
+   
RelationGetRelationName(origRelation),
+   
get_namespace_name(get_session_variable_namespace(varid)),
+   
get_session_variable_name(varid;
}
 
/* Else, ignore dependees that aren't user columns of relations 
*/
-- 
2.37.0

>From 32ebe63cb31457a6f0e5bf06fa99b91393d6d7c0 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Sat, 3 Sep 2022 03:13:47 +0800
Subject: [PATCH 2/3] FIXUP 0002-session-variables

---
 src/b

Re: Can we avoid chdir'ing in resolve_symlinks() ?

[Andrew Dunstan]

On 2022-09-01 Th 19:39, Tom Lane wrote:
> find_my_exec() wants to obtain an absolute, symlink-free path
> to the program's own executable, for what seem to me good
> reasons.  However, chasing down symlinks is left to its
> subroutine resolve_symlinks(), which does this:
>
>  * To resolve a symlink properly, we have to chdir into its directory and
>  * then chdir to where the symlink points; otherwise we may fail to
>  * resolve relative links correctly (consider cases involving mount
>  * points, for example).  After following the final symlink, we use
>  * getcwd() to figure out where the heck we're at.
>
> and then afterwards it has to chdir back to the original cwd.
> That last step is a bit of a sore spot, because sometimes
> (especially in sudo situations) we may not have the privileges
> necessary to do that; I think this is the cause of the complaint
> at [1].  Anyway the whole thing seems a bit excessively Rube
> Goldbergian.  I'm wondering why we couldn't just read the
> symlink(s), concatenate them together, and use canonicalize_path()
> to clean up any mess.
>
> This code was mine originally (336969e49), but I sure don't
> remember why I wrote it like that.  I know we didn't have a
> robust version of canonicalize_path() then, and that may have
> been the main issue, but that offhand comment about mount
> points bothers me.  But I can't reconstruct precisely what
> I was worried about there.  The only contemporaneous discussion
> thread I can find is [2], which doesn't go into coding details.
>
> Thoughts?
>
>   regards, tom lane
>
> [1] 
> https://www.postgresql.org/message-id/flat/CAH8yC8kOj0pmHF1RbK2Gb2t4YCcNG-5h0TwZ7yxk3Hzw6C0Otg%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/flat/4973.1099605411%40sss.pgh.pa.us
>
>

These days there seem to be library functions that do this, realpath(3)
and canonicalize_file_name(3). The latter is what seems to be called by
readlink(1). Should we be using one of those? I don't know how portable
they are. I don't see them here :-(



cheers


andrew



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

pg_basebackup's --gzip switch misbehaves

[Tom Lane]

I've been trying to figure out why my new buildfarm animal mamba
occasionally fails the pg_basebackup tests [1][2].  I've not run
that to ground yet, but one thing I've found that's consistently
reproducible everywhere is that pg_basebackup's --gzip switch
misbehaves.  The manual says, and the principle of least astonishment
agrees, that that should invoke gzip with the default compression
level.  However, the three test cases beginning at about line 810 of
010_pg_basebackup.pl produce these output file sizes on my x86_64
Linux machine:

backup_gzip ("--compress 1"):
total 3672
-rw-r-. 1 postgres postgres  137756 Sep  2 23:38 backup_manifest
-rw-r-. 1 postgres postgres 3538992 Sep  2 23:38 base.tar.gz
-rw-r-. 1 postgres postgres   73991 Sep  2 23:38 pg_wal.tar.gz

backup_gzip2 ("--gzip"):
total 19544
-rw-r-. 1 postgres postgres   137756 Sep  2 23:38 backup_manifest
-rw-r-. 1 postgres postgres  3086972 Sep  2 23:38 base.tar.gz
-rw-r-. 1 postgres postgres 16781399 Sep  2 23:38 pg_wal.tar.gz

backup_gzip3 ("--compress gzip:1"):
total 3672
-rw-r-. 1 postgres postgres  137756 Sep  2 23:38 backup_manifest
-rw-r-. 1 postgres postgres 3539006 Sep  2 23:38 base.tar.gz
-rw-r-. 1 postgres postgres   73989 Sep  2 23:38 pg_wal.tar.gz

It makes sense that base.tar.gz is compressed a little better with
--gzip than with level-1 compression, but why is pg_wal.tar.gz not
compressed at all?  It looks like the problem probably boils down to
which of "-1" and "0" means "default behavior" vs "no compression",
with different code layers interpreting that differently.  I can't
find exactly where that's happening, but I did manage to stop the
failures with this crude hack:

diff --git a/src/bin/pg_basebackup/walmethods.c 
b/src/bin/pg_basebackup/walmethods.c
index e90aa0ba37..e9b578 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -1358,7 +1358,7 @@ CreateWalTarMethod(const char *tarbase,
sprintf(tar_data->tarfilename, "%s%s", tarbase, suffix);
tar_data->fd = -1;
tar_data->compression_algorithm = compression_algorithm;
-   tar_data->compression_level = compression_level;
+   tar_data->compression_level = compression_level > 0 ? compression_level 
: Z_DEFAULT_COMPRESSION;
tar_data->sync = sync;
 #ifdef HAVE_LIBZ
if (compression_algorithm == PG_COMPRESSION_GZIP)

That's not right as a real fix, because it would have the effect
that "--compress gzip:0" would also invoke default compression,
whereas what it should do is produce the uncompressed output
we're actually getting.  Both cases have compression_level == 0
by the time we reach here, though.

I suspect that there are related bugs in other code paths in this
rat's nest of undocumented functions and dubious API abstractions;
but since it's all undocumented, who can say which places are wrong
and which are not?

I might not ding this code quite this hard, if I hadn't had
equally-unpleasant encounters with it previously (eg 248c3a937).
It's a mess, and I do not find it to be up to project standards.

A vaguely-related matter is that the deflateParams calls all pass "0"
as the third parameter:

if (deflateParams(tar_data->zp, tar_data->compression_level, 0) != Z_OK)

Aside from being unreadable, that's entirely unwarranted familiarity
with the innards of libz.  zlib.h says you should be writing a named
constant, probably Z_DEFAULT_STRATEGY.

BTW, I'm fairly astonished that anyone would have thought that three
complete pg_basebackup cycles testing essentially-identical options
were a good use of developer time and buildfarm cycles from here to
eternity.  Even if digging into it did expose a bug, the test case
deserves little credit for that, because it entirely failed to call
attention to the problem.  I had to whack the script pretty hard
just to get it to not delete the evidence.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2022-09-01%2018%3A38%3A27
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2022-08-31%2011%3A46%3A09

Re: Can we avoid chdir'ing in resolve_symlinks() ?

[Tom Lane]

Andrew Dunstan  writes:
> On 2022-09-01 Th 19:39, Tom Lane wrote:
>> find_my_exec() wants to obtain an absolute, symlink-free path
>> to the program's own executable, for what seem to me good
>> reasons.  However, chasing down symlinks is left to its
>> subroutine resolve_symlinks(), which does this:

> These days there seem to be library functions that do this, realpath(3)
> and canonicalize_file_name(3). The latter is what seems to be called by
> readlink(1). Should we be using one of those?

Oh!  I see realpath() in POSIX, but not canonicalize_file_name().
It does look like realpath() would be helpful here, although if
it's not present on Windows that's a problem.

Quick googling suggests that _fullpath() could be used as a substitute.

regards, tom lane

Re: Solaris "sed" versus pre-v13 plpython tests

[Noah Misch]

On Thu, Sep 01, 2022 at 11:10:35AM +0200, Alvaro Herrera wrote:
> On 2022-Aug-31, Noah Misch wrote:
> 
> > On Wed, Aug 31, 2022 at 06:25:01PM -0400, Tom Lane wrote:
> > > I am not completely sure why buildfarm member wrasse isn't failing
> > > similarly
> > 
> > wrasse disabled plpython in v12-, from day one, due to this and a crash bug
> > that I shelved.  I will be interested to see how margay reacts to your fix.
> 
> It turned green two hours ago.  Yay :-)

Excellent.  I can no longer reproduce the crash bug, so I enabled plpython on
wrasse v10,v11,v12.

Re: Can we avoid chdir'ing in resolve_symlinks() ?

[Tom Lane]

I wrote:
> Andrew Dunstan  writes:
>> These days there seem to be library functions that do this, realpath(3)
>> and canonicalize_file_name(3). The latter is what seems to be called by
>> readlink(1). Should we be using one of those?

> Oh!  I see realpath() in POSIX, but not canonicalize_file_name().
> It does look like realpath() would be helpful here, although if
> it's not present on Windows that's a problem.

After some surveying of man pages, I conclude that

(1) realpath() exists on all platforms of interest except Windows,
where it looks like we can use _fullpath() instead.

(2) AIX and Solaris 10 only implement the SUSv2 semantics,
where the caller must supply a buffer that it has no good way
to determine a safe size for.  Annoying.

(3) The Solaris 10 man page has this interesting disclaimer:

 The realpath() function might fail to return to the current
 directory if an error occurs.

which implies that on that platform it's basically implemented
in the same way as our current code.  Sigh.

I think we can ignore (3) though.  Solaris 11 seems to have an
up-to-speed implementation of realpath(), and 10 will be EOL
in January 2024 according to Wikipedia.

As for (2), both systems promise to report EINVAL for a null
pointer, which is also what SUSv2 says.  So I think what we
can do is approximately

ptr = realpath(fname, NULL);
if (ptr == NULL && errno == EINVAL)
{
ptr = pg_malloc(MAXPGPATH);
ptr = realpath(fname, ptr);
}

and just take it on faith that MAXPGPATH is enough on those
platforms.

regards, tom lane

Re: allowing for control over SET ROLE

[Jeff Davis]

On Wed, 2022-08-31 at 08:56 -0400, Robert Haas wrote:
> In some circumstances, it may be desirable to control this behavior.
> For example, if we GRANT pg_read_all_settings TO seer, we do want the
> seer to be able to read all the settings, else we would not have
> granted the role. But we might not want the seer to be able to do
> this:
> 
> You are now connected to database "rhaas" as user "seer".
> rhaas=> set role pg_read_all_settings;
> SET
> rhaas=> create table artifact (a int);
> CREATE TABLE
> rhaas=> \d
>     List of relations
>  Schema |   Name   | Type  |    Owner
> +--+---+--
>  public | artifact | table | pg_read_all_settings
> (1 row)

Interesting case.

> I have attached a rather small patch which makes it possible to
> control this behavior:
> 
> You are now connected to database "rhaas" as user "rhaas".
> rhaas=# grant pg_read_all_settings to seer with set false;
> GRANT ROLE

You've defined this in terms of the mechanics -- allow SET ROLE or not
-- but I assume you intend it as a security feature to allow/forbid
some capabilities.

Is this only about the capability to create objects owned by a role
you're a member of? Or are there other implications?

Regards,
Jeff Davis

Re: Fix typo function circle_same (src/backend/utils/adt/geo_ops.c)

[Daniel Gustafsson]

> On 3 Sep 2022, at 09:36, Julien Rouhaud  wrote:

> Yeah, there are unfortunately a lot of problems around those and NaN, with
> multiple reports in the past (I recall [1] and [2] but there were others).

NaNs are indeed incredibly complicated, but I think we are sort of in a good
place here given it's testing for equality in floats.  The commit message of
c4c34008854654279ec30067d72fc5d174d2f42f carries an explanation:

The float datatypes consider NaNs values to be equal and greater than
all non-NaN values.  This change considers NaNs equal only for equality
operators.  The placement operators, contains, overlaps, left/right of
etc.  continue to return false when NaNs are involved.

From testing and reading I believe the fix in this thread is correct, but since
NaNs are involved I will take another look at this with fresh eyes before going
ahead.

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

freeing LDAPMessage in CheckLDAPAuth

[Zhihong Yu]

Hi,
In CheckLDAPAuth(), around line 2606:

if (r != LDAP_SUCCESS)
{
ereport(LOG,
(errmsg("could not search LDAP for filter \"%s\" on
server \"%s\": %s",

It seems that the call to ldap_msgfree() is missing in the above case.
According to
https://www.openldap.org/software//man.cgi?query=ldap_search_s&sektion=3&apropos=0&manpath=OpenLDAP+2.4-Release
:

   Note  that  *res*  parameter  of  *ldap*_*search*_*ext*_*s()*
and *ldap*_*search*_*s()*
   should be freed with *ldap*_*msgfree()* regardless of return
value of these
   functions.

Please see the attached patch which frees the search_message in the above case.


Thanks


ldap-msg-free.patch
Description: Binary data

Re: warning: comparison of integer expressions of different signedness related to simd.h

[John Naylor]

On Sat, Sep 3, 2022 at 12:57 PM Tom Lane  wrote:
>
> John Naylor  writes:
> > On Sat, Sep 3, 2022 at 12:30 PM Pavel Stehule  
> > wrote:
> >> /usr/local/pgsql/master/include/server/port/simd.h: In function 
> >> ‘vector8_has’:
> >> /usr/local/pgsql/master/include/server/port/simd.h:168:27: warning: 
> >> comparison of integer expressions of different signedness: ‘int’ and ‘long 
> >> unsigned int’ [-Wsign-compare]
> >> 168 | for (int i = 0; i < sizeof(Vector8); i++)
>
> > "int" should probably be "Size" -- does that remove the warning?
>
> Agreed, should be Size or size_t, or else cast the sizeof() result.
> But I wonder why none of the buildfarm is showing such a warning.

If I add -Wsign-compare to CPPFLAGS, I get dozens of warnings all over
the place. It's probably unreasonable for extensions to expect to
compile cleanly with warnings that the core server doesn't use, but
this header is clearly wrong and easy to remedy, so I've pushed a
patch.

-- 
John Naylor
EDB: http://www.enterprisedb.com

Re: Can we avoid chdir'ing in resolve_symlinks() ?

[Tom Lane]

Here's a draft patch for this.  It seems to work on Linux,
but the Windows code is just speculation.  In particular,
I did

path = _fullpath(NULL, fname, 0);
if (path == NULL)
_dosmaperr(GetLastError());

but I'm not really sure that the _dosmaperr bit is needed,
because the _fullpath man page I found makes reference to
setting "errno" [1].  It's likely to be hard to test, because
most of the possible error cases should be nigh unreachable
in our usage; we already know the input is a valid reference
to an executable file.

BTW, I noticed what seems a flat-out bug in validate_exec:

/* Win32 requires a .exe suffix for stat() */
-   if (strlen(path) >= strlen(".exe") &&
+   if (strlen(path) < strlen(".exe") ||
pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 
0)

Nobody's noticed because none of our executables have base names
shorter than 4 characters, but it's still a bug.

regards, tom lane

[1] 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fullpath-wfullpath?view=msvc-170

diff --git a/src/common/exec.c b/src/common/exec.c
index 22f04aafbe..fa01ff7b32 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -14,6 +14,15 @@
  *-
  */
 
+/*
+ * On macOS, "man realpath" avers:
+ *Defining _DARWIN_C_SOURCE or _DARWIN_BETTER_REALPATH before including
+ *stdlib.h will cause the provided implementation of realpath() to use
+ *F_GETPATH from fcntl(2) to discover the path.
+ * This should be harmless everywhere else.
+ */
+#define _DARWIN_BETTER_REALPATH
+
 #ifndef FRONTEND
 #include "postgres.h"
 #else
@@ -58,11 +67,8 @@ extern int	_CRT_glob = 0;		/* 0 turns off globbing; 1 turns it on */
 	(fprintf(stderr, __VA_ARGS__), fputc('\n', stderr))
 #endif
 
-#ifdef _MSC_VER
-#define getcwd(cwd,len)  GetCurrentDirectory(len, cwd)
-#endif
-
-static int	resolve_symlinks(char *path);
+static int	normalize_exec_path(char *path);
+static char *pg_realpath(const char *fname);
 
 #ifdef WIN32
 static BOOL GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser);
@@ -87,7 +93,7 @@ validate_exec(const char *path)
 	char		path_exe[MAXPGPATH + sizeof(".exe") - 1];
 
 	/* Win32 requires a .exe suffix for stat() */
-	if (strlen(path) >= strlen(".exe") &&
+	if (strlen(path) < strlen(".exe") ||
 		pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
 	{
 		strlcpy(path_exe, path, sizeof(path_exe) - 4);
@@ -151,30 +157,16 @@ validate_exec(const char *path)
 int
 find_my_exec(const char *argv0, char *retpath)
 {
-	char		cwd[MAXPGPATH],
-test_path[MAXPGPATH];
 	char	   *path;
 
-	if (!getcwd(cwd, MAXPGPATH))
-	{
-		log_error(errcode_for_file_access(),
-  _("could not identify current directory: %m"));
-		return -1;
-	}
-
 	/*
 	 * If argv0 contains a separator, then PATH wasn't used.
 	 */
-	if (first_dir_separator(argv0) != NULL)
+	strlcpy(retpath, argv0, MAXPGPATH);
+	if (first_dir_separator(retpath) != NULL)
 	{
-		if (is_absolute_path(argv0))
-			strlcpy(retpath, argv0, MAXPGPATH);
-		else
-			join_path_components(retpath, cwd, argv0);
-		canonicalize_path(retpath);
-
 		if (validate_exec(retpath) == 0)
-			return resolve_symlinks(retpath);
+			return normalize_exec_path(retpath);
 
 		log_error(errcode(ERRCODE_WRONG_OBJECT_TYPE),
   _("invalid binary \"%s\": %m"), retpath);
@@ -183,9 +175,8 @@ find_my_exec(const char *argv0, char *retpath)
 
 #ifdef WIN32
 	/* Win32 checks the current directory first for names without slashes */
-	join_path_components(retpath, cwd, argv0);
 	if (validate_exec(retpath) == 0)
-		return resolve_symlinks(retpath);
+		return normalize_exec_path(retpath);
 #endif
 
 	/*
@@ -208,21 +199,15 @@ find_my_exec(const char *argv0, char *retpath)
 			if (!endp)
 endp = startp + strlen(startp); /* point to end */
 
-			strlcpy(test_path, startp, Min(endp - startp + 1, MAXPGPATH));
+			strlcpy(retpath, startp, Min(endp - startp + 1, MAXPGPATH));
 
-			if (is_absolute_path(test_path))
-join_path_components(retpath, test_path, argv0);
-			else
-			{
-join_path_components(retpath, cwd, test_path);
-join_path_components(retpath, retpath, argv0);
-			}
+			join_path_components(retpath, retpath, argv0);
 			canonicalize_path(retpath);
 
 			switch (validate_exec(retpath))
 			{
 case 0:			/* found ok */
-	return resolve_symlinks(retpath);
+	return normalize_exec_path(retpath);
 case -1:		/* wasn't even a candidate, keep looking */
 	break;
 case -2:		/* found but disqualified */
@@ -241,105 +226,90 @@ find_my_exec(const char *argv0, char *retpath)
 
 
 /*
- * resolve_symlinks - resolve symlinks to the underlying file
+ * normalize_exec_path - resolve symlinks and convert to absolute path
  *
- * Replace "path" by the absolute path to the referenced file.
+ * Given a path that refers to an executable, chase through any symlinks
+ * to find the real file loca

Re: Add tracking of backend memory allocated to pg_stat_activity

[Reid Thompson]

On Wed, 2022-08-31 at 12:05 -0500, Justin Pryzby wrote:
> > +  proargmodes =>
> > '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}'
> > ,
> 
> In the past, there was concern about making pg_stat_activity wider by
> adding information that's less-essential than what's been there for
> years.  This is only an int64, so it's not "wide", but I wonder if
> there's another way to expose this information?  Like adding backends
> to
> pg_get_backend_memory_contexts() , maybe with another view on top of

I will take a look at pg_get_backend_memory_contexts. I will also look
at the other suggestions in the thread.

> +    * shown allocated in pgstat_activity when the
> 
> pg_stat

Corrected,

> > replacing the * old
> 
> wrapping caused extraneous stars

Corrected

> > here asit's
> 
> as it's

Corrected

> errmsg() doesn't require the outside paranthesis since a couple years
> ago.

Corrected

> > > mem_allocated cast to
> add a comma before "cast" ?

Corrected

Patch with the corrections attached

-- 
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

reid.thomp...@crunchydata.com
www.crunchydata.com


-- 
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

reid.thomp...@crunchydata.com
www.crunchydata.com

-- 
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

reid.thomp...@crunchydata.com
www.crunchydata.com
From 584a04f1b53948049e73165a4ffdd544c950ab0d Mon Sep 17 00:00:00 2001
From: Reid Thompson 
Date: Thu, 11 Aug 2022 12:01:25 -0400
Subject: [PATCH] Add tracking of backend memory allocated to pg_stat_activity

This new field displays the current bytes of memory allocated to the
backend process. It is updated as memory for the process is
malloc'd/free'd. Memory allocated to items on the freelist is included in
the displayed value.  Dynamic shared memory allocations are included
only in the value displayed for the backend that created them, they are
not included in the value for backends that are attached to them to
avoid double counting. On occasion, orphaned memory segments may be
cleaned up on postmaster startup. This may result in decreasing the sum
without a prior increment. We limit the floor of backend_mem_allocated
to zero. Updated pg_stat_activity documentation for the new column.
---
 doc/src/sgml/monitoring.sgml|  12 +++
 src/backend/catalog/system_views.sql|   1 +
 src/backend/storage/ipc/dsm_impl.c  |  80 +++
 src/backend/utils/activity/backend_status.c | 105 
 src/backend/utils/adt/pgstatfuncs.c |   4 +-
 src/backend/utils/mmgr/aset.c   |  18 
 src/backend/utils/mmgr/generation.c |  15 +++
 src/backend/utils/mmgr/slab.c   |  21 
 src/include/catalog/pg_proc.dat |   6 +-
 src/include/utils/backend_status.h  |   7 +-
 src/test/regress/expected/rules.out |   9 +-
 11 files changed, 269 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1d9509a2f6..40ae638f25 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -947,6 +947,18 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  
 
 
+
+ 
+  backend_mem_allocated bigint
+ 
+ 
+  The byte count of memory allocated to this backend. Dynamic shared memory
+  allocations are included only in the value displayed for the backend that
+  created them, they are not included in the value for backends that are
+  attached to them to avoid double counting.
+ 
+
+
  
   
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5a844b63a1..d23f0e9dbb 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -863,6 +863,7 @@ CREATE VIEW pg_stat_activity AS
 S.backend_xid,
 s.backend_xmin,
 S.query_id,
+S.backend_mem_allocated,
 S.query,
 S.backend_type
 FROM pg_stat_get_activity(NULL) AS S
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index e1b90c5de4..3356bb65b5 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -66,6 +66,7 @@
 #include "postmaster/postmaster.h"
 #include "storage/dsm_impl.h"
 #include "storage/fd.h"
+#include "utils/backend_status.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
 
@@ -232,6 +233,13 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
 			name)));
 			return false;
 		}
+		/*
+		 * Detach and destroy pass through here, only decrease the memory
+		 * shown allocated in pg_stat_activity when the creator destroys the
+		 * allocation.
+		 */
+		if (op == DSM_OP_DESTROY)
+			pgstat_report_backend_mem_allocated_decrease(*mapped_size);
 		*mapped_address = NULL;
 		*mapped_size = 0;
 		if (op == DSM_OP_

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[Reid Thompson]

On Wed, 2022-08-31 at 12:34 -0500, Justin Pryzby wrote:
> You should name the patches with different prefixes, like
> 001,002,003  Otherwise, cfbot may try to apply them in the wrong
> order.
> git format-patch is the usual tool for that.

Thanks for the pointer. My experience with git in the past has been
minimal and basic.

> > +    Specifies a limit to the amount of memory (MB) that may be
> > allocated to
> 
> MB are just the default unit, right ?
> The user should be allowed to write max_total_backend_memory='2GB'

Correct. Default units are MB. Other unit types are converted to MB.

> > +    causing that backends current query/transaction to fail.
> 
> backend's
> > +    allocated exceeding the limit. Further requests will not
> 
> allocated, and exceed the limit
> 
> > +   if (MyAuxProcType != NotAnAuxProcess)
> The double negative is confusing, so could use a comment.

> > +   elog(WARNING,
> I think it should be ereport() rather than elog(), which is
> internal-only, and not-translated.

Corrected/added the the above items. Attached patches with the corrections.

> > +   0, 0, INT_MAX,
> > +   NULL, NULL, NULL
> I think this needs a maximum like INT_MAX/1024/1024

Is this noting that we'd set a ceiling of 2048MB?

> > +   for (i = 1; i <= NumBackendStatSlots; i++)
> > +   {
> 
> It's looping over every backend for each allocation.
> Do you know if there's any performance impact of that ?

I'm not very familiar with how to test performance impact, I'm open to
suggestions.  I have performed the below pgbench tests and noted the basic
tps differences in the table.

Test 1:
branch master
CFLAGS="-I/usr/include/python3.8/ " /home/rthompso/src/git/postgres/configure 
--silent --prefix=/home/rthompso/src/git/postgres/install/master --with-openssl 
--with-tcl --with-tclconfig=/usr/lib/tcl8.6 --with-perl --with-libxml 
--with-libxslt --with-python --with-gssapi --with-systemd --with-ldap 
--enable-nls
make -s -j12 && make -s install
initdb
default postgresql.conf settings
init pgbench pgbench -U rthompso -p 5433 -h localhost -i -s 50 testpgbench
10 iterations
for ctr in {1..10}; do { time pgbench  -p 5433 -h localhost -c 10 -j 10 -t 
5 testpgbench; } 2>&1 | tee -a pgstatsResultsNoLimitSet; done

Test 2:
branch pg-stat-activity-backend-memory-allocated
CFLAGS="-I/usr/include/python3.8/ " /home/rthompso/src/git/postgres/configure 
--silent --prefix=/home/rthompso/src/git/postgres/install/pg-stats-memory/ 
--with-openssl --with-tcl --with-tclconfig=/usr/lib/tcl8.6 --with-perl 
--with-libxml --with-libxslt --with-python --with-gssapi --with-systemd 
--with-ldap --enable-nls
make -s -j12 && make -s install
initdb 
default postgresql.conf settings
init pgbench pgbench -U rthompso -p 5433 -h localhost -i -s 50
testpgbench
10 iterations
for ctr in {1..10}; do { time pgbench  -p 5433 -h localhost -c 10  -j 10 -t 
5 testpgbench; } 2>&1 | tee -a pgstatsResultsPg-stats-memory; done

Test 3:
branch dev-max-memory
CFLAGS="-I/usr/include/python3.8/ " /home/rthompso/src/git/postgres/configure 
--silent --prefix=/home/rthompso/src/git/postgres/install/dev-max-memory/ 
--with-openssl --with-tcl --with-tclconfig=/usr/lib/tcl8.6 --with-perl 
--with-libxml --with-libxslt --with-python --with-gssapi --with-systemd 
--with-ldap --enable-nls
make -s -j12 && make -s install
initdb 
default postgresql.conf settings
init pgbench pgbench -U rthompso -p 5433 -h localhost -i -s 50 testpgbench
10 iterations
for ctr in {1..10}; do { time pgbench  -p 5433 -h localhost -c 10  -j 10 -t 
5 testpgbench; } 2>&1 | tee -a pgstatsResultsDev-max-memory; done

Test 4:
branch dev-max-memory
CFLAGS="-I/usr/include/python3.8/ " /home/rthompso/src/git/postgres/configure 
--silent --prefix=/home/rthompso/src/git/postgres/install/dev-max-memory/ 
--with-openssl --with-tcl --with-tclconfig=/usr/lib/tcl8.6 --with-perl 
--with-libxml --with-libxslt --with-python --with-gssapi --with-systemd 
--with-ldap --enable-nls
make -s -j12 && make -s install
initdb 
non-default postgresql.conf setting for max_total_backend_memory = 100MB
init pgbench pgbench -U rthompso -p 5433 -h localhost -i -s 50 testpgbench
10 iterations
for ctr in {1..10}; do { time pgbench  -p 5433 -h localhost -c 10  -j 10 -t 
5 testpgbench; } 2>&1 | tee -a pgstatsResultsDev-max-memory100MB; done

Laptop
11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz 8 Cores 16 threads
32GB RAM
SSD drive

Averages from the 10 runs and tps difference over the 10 runs
|--+--++---+--+---+---+--|
| Test Run | Master   | Track Memory Allocated | Diff from 
Master  | Max Mem off  | Diff from Master  | Max Mem 100MB | Diff from 
Master |
| Set 1| Test 1   | Test 2 |
   | Test 3   |   | Test 4|   

Re: making relfilenodes 56 bits

[Dilip Kumar]

On Sat, Sep 3, 2022 at 5:11 PM Amit Kapila  wrote:

> > I have found one more issue with this approach of rewriting the
> > conflicting table.  Earlier I thought we could do the conflict
> > checking and rewriting inside create_new_objects() right before the
> > restore command.  But after implementing (while testing) this I
> > realized that we DROP and CREATE the database while restoring the dump
> > that means it will again generate the conflicting system tables.  So
> > theoretically the rewriting should go in between the CREATE DATABASE
> > and restoring the object but as of now both create database and
> > restoring other objects are part of a single dump file.  I haven't yet
> > analyzed how feasible it is to generate the dump in two parts, first
> > part just to create the database and in second part restore the rest
> > of the object.
> >
>
> Isn't this happening because we are passing "--clean
> --create"/"--create" options to pg_restore in create_new_objects()? If
> so, then I think one idea to decouple would be to not use those
> options. Perform drop/create separately via commands (for create, we
> need to generate the command as we are generating while generating the
> dump in custom format), then rewrite the conflicting tables, and
> finally restore the dump.

Hmm, you are right.  So I think something like this is possible to do,
I will explore this more. Thanks for the idea.

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

Re: Add 64-bit XIDs into PostgreSQL 15

[Dilip Kumar]

On Mon, Jul 18, 2022 at 2:54 PM Pavel Borisov  wrote:
>>
>> > I can agree with you that sending rebased patches too often can be a 
>> > little annoying. On the other hand, otherwise, it's just red in Cfbot. I 
>> > suppose it's much easier and more comfortable to review the patches that 
>> > at least apply cleanly and pass all tests. So if Cfbot is red for a long 
>> > time I feel we need to send a rebased patchset anyway.
>> >
>> > I'll try to not doing this too often but frankly, I don't see a better 
>> > alternative at the moment.
>>
>> Considering the overall activity on the mailing list personally I
>> don't see a problem here. Several extra emails don't bother me at all,
>> but I would like to see a green cfbot report for an open item in the
>> CF application. Otherwise someone will complain that the patch doesn't
>> apply anymore and the result will be the same as for sending an
>> updated patch, except that we will receive at least two emails instead
>> of one.
>
> Hi, Alexander!
> Agree with you. I also consider green cfbot entry important. So PFA rebased 
> v43.

Since we have converted TransactionId to 64-bit, so do we still need
the concept of FullTransactionId?  I mean it is really confusing to
have 3 forms of transaction ids.  i.e. Transaction Id,
FullTransactionId and ShortTransactionId.

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

Re: Schema variables - new implementation for Postgres 15

[Julien Rouhaud]

On Sat, Sep 03, 2022 at 11:00:51PM +0800, Julien Rouhaud wrote:
> Hi,
>
> On Fri, Sep 02, 2022 at 07:42:00AM +0200, Pavel Stehule wrote:
> >
> > rebased today
>
> After some off-list discussion with Pavel, I'm sending some proposal patches
> (in .txt extension) to apply to the last patchset.
>
> To sum up, when a session issues a DROP VARIABLE, the session will receive an
> sinval notification for its own drop and we don't want to process it
> immediately as we need to hold the value in case the transaction is 
> rollbacked.
> The current patch avoided that by forcing a single processing of sinval per
> transaction, and forcing it before dropping the variable.  It works but it
> seems to me that postponing all but the first VARIABLEOID sinval to the next
> transaction is a big hammer, and the sooner we can free some memory the 
> better.
>
> For an alternative approach the attached patch store the lxid in the SVariable
> itself when dropping a currently set variable, so we can process all sinval 
> and
> simply defer to the next transaction the memory cleanup of the variable(s) we
> know we just dropped.  What do you think of that approach?
>
> As I was working on some changes I also made a pass on session_variable.c.  I
> tried to improve a bit some comments, and also got rid of the "first_time"
> variable.  The name wasn't really great, and AFAICS it can be replaced by
> testing whether the memory context has been created yet or not.
>
> But once that done I noticed the get_rowtype_value() function.  I don't think
> this function is necessary as the core code already knows how to deal with
> stored datum when the underlying composite type was modified.  I tried to
> bypass that function and always simply return the stored value and all the
> tests run fine.  Is there really any cases when this extra code is needed?
>
> FTR I tried to do a bunch of additional testing using relation as base type 
> for
> variable, as you can do more with those than plain composite types, but it
> still always works just fine.
>
> However, while doing so I noticed that find_composite_type_dependencies()
> failed to properly handle dependencies on relation (plain tables, matviews and
> partitioned tables).  I'm also adding 2 additional patches to fix this corner
> case and add an additional regression test for the plain table case.

I forgot to mention this chunk:

+   /*
+* Although the value of domain type should be valid (it is
+* checked when it is assigned to session variable), we have to
+* check related constraints anytime. It can be more expensive
+* than in PL/pgSQL. PL/pgSQL forces domain checks when value
+* is assigned to the variable or when value is returned from
+* function. Fortunately, domain types manage cache of constraints by
+* self.
+*/
+   if (svar->is_domain)
+   {
+   MemoryContext oldcxt = CurrentMemoryContext;
+
+   /*
+* Store domain_check extra in CurTransactionContext. When we 
are
+* in other transaction, the domain_check_extra cache is not 
valid.
+*/
+   if (svar->domain_check_extra_lxid != MyProc->lxid)
+   svar->domain_check_extra = NULL;
+
+   domain_check(svar->value, svar->isnull,
+svar->typid, &svar->domain_check_extra,
+CurTransactionContext);
+
+   svar->domain_check_extra_lxid = MyProc->lxid;
+
+   MemoryContextSwitchTo(oldcxt);
+   }

I agree that storing the domain_check_extra in the transaction context sounds
sensible, but the memory context handling is not quite right.

Looking at domain_check, it doesn't change the current memory context, so as-is
all the code related to oldcxt is unnecessary.

Some other callers like expandedrecord.c do switch to a short lived context to
limit the lifetime of the possible leak by the expression evaluation, but I
don't think that's an option here.

Re: build remaining Flex files standalone

[John Naylor]

On Sat, Sep 3, 2022 at 1:29 AM Andres Freund  wrote:

> > Subject: [PATCH v4 01/11] Preparatory refactoring for compiling guc-file.c
> >  standalone
> > Subject: [PATCH v4 02/11] Move private declarations shared between guc.c and
> >  guc-file.l to new header
> > Subject: [PATCH v4 03/11] Build guc-file.c standalone
>
> 01-03 are a bit more complicated, but still look not far off. There's a FIXME
> about failing headercheck.

Fixed by adding utils/guc.h to the new internal header, which now
lives in the same directory as guc.c and guc-file.l, similar to how I
did json path in the last patch. Also removed the bogus include from
v4 to . Pushed 01 and 02 separately, then squashed and pushed the
rest.


--
John Naylor
EDB: http://www.enterprisedb.com

Re: pg_basebackup's --gzip switch misbehaves

[Michael Paquier]

On Sat, Sep 03, 2022 at 11:11:29AM -0400, Tom Lane wrote:
> It makes sense that base.tar.gz is compressed a little better with
> --gzip than with level-1 compression, but why is pg_wal.tar.gz not
> compressed at all?  It looks like the problem probably boils down to
> which of "-1" and "0" means "default behavior" vs "no compression",
> with different code layers interpreting that differently.  I can't
> find exactly where that's happening, but I did manage to stop the
> failures with this crude hack:

There is a distinction coming in pg_basebackup.c from the way we
deparse the compression specification and the default compression
level that should be assigned if there is no level directly specified
by the user.  It seems to me that the error comes from this code in
BaseBackup() when we are under STREAM_WAL (default):

if (client_compress->algorithm == PG_COMPRESSION_GZIP)
{
wal_compress_algorithm = PG_COMPRESSION_GZIP;
wal_compress_level =
   (client_compress->options & PG_COMPRESSION_OPTION_LEVEL)
   != 0 ? client_compress->level : 0;

ffd5365 has missed that wal_compress_level should be set to
Z_DEFAULT_COMPRESSION if there is nothing set in the compression
spec for a zlib build.  pg_receivewal.c enforces that already.

> That's not right as a real fix, because it would have the effect
> that "--compress gzip:0" would also invoke default compression,
> whereas what it should do is produce the uncompressed output
> we're actually getting.  Both cases have compression_level == 0
> by the time we reach here, though.

Nope, that would not be right.

> BTW, I'm fairly astonished that anyone would have thought that three
> complete pg_basebackup cycles testing essentially-identical options
> were a good use of developer time and buildfarm cycles from here to
> eternity.  Even if digging into it did expose a bug, the test case
> deserves little credit for that, because it entirely failed to call
> attention to the problem.  I had to whack the script pretty hard
> just to get it to not delete the evidence.

The introduction of the compression specification has introduced a lot
of patterns where we expect or not expect compression to happen, and
on top of that this needs to be careful about backward-compatibility.
--
Michael


signature.asc
Description: PGP signature

Re: freeing LDAPMessage in CheckLDAPAuth

[Michael Paquier]

On Sat, Sep 03, 2022 at 05:00:30PM -0700, Zhihong Yu wrote:
>Note  that  *res*  parameter  of  *ldap*_*search*_*ext*_*s()*
> and *ldap*_*search*_*s()*
>should be freed with *ldap*_*msgfree()* regardless of return
> value of these
>functions.
> 
> Please see the attached patch which frees the search_message in the above 
> case.

Yep, nice catch, I am reading the same thing as you do.  I can see
that we already do that after a failing ldap_search_st() call in
fe-connect.c for libpq.  Hence, similarly, we'd better call
ldap_msgfree() on search_message when it is not NULL after a search
failure, no?  The patch you are proposing does not do that.
--
Michael


signature.asc
Description: PGP signature

Re: json docs fix jsonb_path_exists_tz again

[Michael Paquier]

On Sat, Sep 03, 2022 at 10:01:25AM -0400, Andrew Dunstan wrote:
> Thanks for fixing, you beat me to it.

No problem, I was just passing by :)
--
Michael


signature.asc
Description: PGP signature

Re: freeing LDAPMessage in CheckLDAPAuth

[Tom Lane]

Michael Paquier  writes:
> On Sat, Sep 03, 2022 at 05:00:30PM -0700, Zhihong Yu wrote:
>> Please see the attached patch which frees the search_message in the above 
>> case.

> Yep, nice catch, I am reading the same thing as you do.  I can see
> that we already do that after a failing ldap_search_st() call in
> fe-connect.c for libpq.  Hence, similarly, we'd better call
> ldap_msgfree() on search_message when it is not NULL after a search
> failure, no?  The patch you are proposing does not do that.

I can't get too excited about this.  All of the error exit paths in
backend authentication code will lead immediately to process exit, so
the possibility of some memory being leaked really has no consequences
worth worrying about.  If we *were* worried about it, sprinkling a few
more ldap_msgfree() calls into the existing code would hardly make it
more bulletproof.  There's lots of psprintf() and other
Postgres-universe calls in that code that could potentially fail and
force an elog exit without reaching ldap_msgfree.  So if you wanted to
make this completely clean you'd need to resort to doing the freeing
in PG_CATCH blocks ... and I don't see any value in hacking it to that
extent.

What might be worth inspecting is the code paths in frontend libpq
that call ldap_msgfree(), because on the client side we don't get to
assume that an error will lead to immediate process exit.  If we've
missed any cleanups over there, that *would* be worth fixing.

regards, tom lane

Re: [RFC] building postgres with meson - v12

[John Naylor]

On Fri, Sep 2, 2022 at 11:35 PM Andres Freund  wrote:
>
> Hi,
>
> On 2022-09-02 14:17:26 +0700, John Naylor wrote:
> > On Thu, Sep 1, 2022 at 1:12 AM Andres Freund  wrote:
> > > [v12]
> >
> > +# Build a small utility static lib for the parser. This makes it easier to 
> > not
> > +# depend on gram.h already having been generated for most of the other code
> > +# (which depends on generated headers having been generated). The 
> > generation
> > +# of the parser is slow...
> >
> > It's not obvious whether this is intended to be a Meson-only
> > optimization or a workaround for something awkward to specify.
>
> It is an optimization. The parser generation is by far the slowest part of a
> build. If other files can only be compiled once gram.h is generated, there's a
> long initial period where little can happen. So instead of having all .c files
> have a dependency on gram.h having been generated, the above makes only
> scan.c, gram.c compilation depend on gram.h.  It only matters for the first
> compilation, because such dependencies are added as order-only dependencies,
> supplanted by more precise compiler generated dependencies after.

Okay, I think the comment could include some of this info for clarity.

> It's still pretty annoying that so much of the build is initially idle,
> waiting for genbki.pl to finish.
>
> Part of that is due to some ugly dependencies of src/common on backend headers
> that IMO probably shouldn't exist (e.g. src/common/relpath.c includes
> catalog/pg_tablespace_d.h).

Technically, *_d.h headers are not backend, that's why it's safe to
include them anywhere. relpath.c in its current form has to know the
tablespace OIDs, which I guess is what you think is ugly. (I agree
it's not great)

> Looks like it'd not be hard to get at least the
> _shlib version of src/common and libpq build without waiting for that. But for
> all the backend code I don't really see a way, so it'd be nice to make genbki
> faster at some point.

The attached gets me a ~15% reduction in clock time by having
Catalog.pm parse the .dat files in one sweep, when we don't care about
formatting, i.e. most of the time:

master:
User time (seconds): 0.48
Maximum resident set size (kbytes): 36112

patch:
User time (seconds): 0.41
Maximum resident set size (kbytes): 35808

That's pretty simple -- I think going beyond that would require some
perl profiling.

-- 
John Naylor
EDB: http://www.enterprisedb.com
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e91a8e10a8..9dd932e30a 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -287,6 +287,8 @@ sub ParseData
 	my $catname = $1;
 	my $data= [];
 
+	if ($preserve_formatting)
+	{
 	# Scan the input file.
 	while (<$ifd>)
 	{
@@ -346,11 +348,24 @@ sub ParseData
 		{
 			push @$data, $hash_ref if !$hash_ref->{autogenerated};
 		}
-		elsif ($preserve_formatting)
+		else
 		{
 			push @$data, $_;
 		}
 	}
+	}
+	else
+	{
+		# When we only care about the contents, it's faster to read and eval
+		# the whole file at once.
+		my $full_file = do { local(@ARGV, $/) = $input_file; <> };
+		eval '$data = ' . $full_file;
+		foreach my $hash_ref (@{$data})
+		{
+			AddDefaultValues($hash_ref, $schema, $catname);
+		}
+	}
+
 	close $ifd;
 
 	# If this is pg_type, auto-generate array types too.