Re: WIP Patch: Add a function that returns binary JSONB as a bytea

[John Naylor]

On Thu, Jun 23, 2022 at 9:06 PM Andres Freund  wrote:
> It looks like there's quite a bit of low hanging fruits to optimize...

Yeah, if escapes and control characters are rare, adding an SSE2 fast
path would give a boost to json_lex_string: check 16 bytes at a time
for those chars (plus the ending double-quote). We can also shave a
few percent by having pg_utf8_verifystr use SSE2 for the ascii path. I
can look into this.

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

Re: Postgres do not allow to create many tables with more than 63-symbols prefix

[Önder Kalacı]

Hi,

Thanks for working on this.

 >> According to subj you can try to create many tables (induced by the case
>  >> of partitioned table) with long prefix - see 6727v.sql for
> reproduction.
>  >> But now it's impossible because of logic of the makeUniqueTypeName()
>  >> routine.
>  >> You get the error:
>  >> ERROR:  could not form array type name for type ...
>  >>
>  >> It is very corner case, of course. But solution is easy and short. So,
>  >> why not to fix? - See the patch in attachment.
>  >
>  > While this seems to be a good improvement, I think it's not a bug.
>  > Probably we cannot backpatch it as it will end up having type names
>  > defined by different naming rules. I'd suggest discussing it on
>  > -hackers.
> Done.
>

On Citus extension, we hit a similar issue while creating partitions (over
multiple transactions in parallel). You can see some more discussions on
the related Github issue #5334
. We basically discuss this
behavior on the issue.

I tested this patch with the mentioned issue, and as expected the issue is
resolved.

Also, in general, the patch looks reasonable, following the approach
that ChooseRelationName()
implements makes sense to me as well.

Onder KALACI
Developing the Citus extension @Microsoft

Future Postgres 15 and Clang 15

[Fabien COELHO]

Just a note/reminder that "seawasp" has been unhappy for some days now 
because of yet another change in the unstable API provided by LLVM:


https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2022-06-23%2023%3A18%3A17

 llvmjit.c:1115:50: error: use of undeclared identifier 'LLVMJITCSymbolMapPair'
LLVMOrcCSymbolMapPairs symbols = palloc0(sizeof(LLVMJITCSymbolMapPair) 
* LookupSetSize);

 llvmjit.c:1233:81: error: too few arguments to function call, expected 3, have 
2
ref_gen = 
LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);

The question is: should pg 15 try to be clang 15 ready? I'm afraid yes, as 
LLVM does 2 releases per year, so clang 15 should come out this Fall, 
together with pg 15. Possibly other changes will come before the 
releases:-/


--
Fabien.

[PATCH] Optimize json_lex_string by batching character copying

[Jelte Fennema]

When parsing JSON strings need to be converted from the JSON string
format to a c-style string. A simple copy of the buffer does not suffice
because of the various escape sequences that that JSON supports. Because
of this our JSON parser wrote characters into the c-style string buffer
one at a time.

However, this is only necessary for these escaped sequences that map to
another character. This patch changes the behaviour for non-escaped
characters. These are now copied in batches instead of one character at
a time.

To test performance of this change I used COPY BINARY from a JSONB table
into another, containing fairly JSONB values of ~15kB. The JSONB values
are a JSON object with a single level. They contain a few small keys and
values, but one very big value that's a stringified JSON blob. So this
JSON blob contains a relatively high number of escape characters, to
escape all the " characters. This change improves performance for
workload this workload on my machine by ~18% (going from 1m24s to 1m09s).

@Andres, there was indeed some low hanging fruit. 
@John Naylor, SSE2 indeed sounds like another nice improvement. I'll leave 
that to you.


0001-Optimize-json_lex_string-by-batching-character-copie.patch
Description:  0001-Optimize-json_lex_string-by-batching-character-copie.patch

Re: Fix instability in subscription regression test

[Amit Kapila]

On Fri, Jun 24, 2022 at 8:07 AM houzj.f...@fujitsu.com
 wrote:
>
> >
> > +# wait for initial table synchronization to finish
> > +$node_subscriber->poll_query_until('postgres', $synced_query)
> >
> > We could probably slightly change the comment to say: "wait for table sync 
> > to
> > finish". Normally, we use initial table sync after CREATE SUBSCRIPTION. 
> > This is a
> > minor nitpick and I can take care of it before committing unless you think
> > otherwise.
>
> Thanks for reviewing, the suggestion looks good to me.
>

Pushed!

-- 
With Regards,
Amit Kapila.

Re: tablesync copy ignores publication actions

[Amit Kapila]

On Fri, Jun 24, 2022 at 2:09 AM Robert Haas  wrote:
>
> On Thu, Jun 23, 2022 at 2:13 AM Amit Kapila  wrote:
> > The patch looks good to me as well. I will push this patch in HEAD (as
> > per option (a)) tomorrow unless I see any more suggestions/comments.
>
> The example seems to demonstrate the point quite well but one thing
> that I notice is that it is quite long. I don't really see an obvious
> way of making it shorter without making it less clear, so perhaps
> that's fine.
>

Thanks for looking into it. Pushed!

-- 
With Regards,
Amit Kapila.

Re: Support logical replication of DDLs

[Amit Kapila]

On Fri, Jun 24, 2022 at 8:10 AM Masahiko Sawada  wrote:
>
> On Thu, Jun 23, 2022 at 7:00 PM Amit Kapila  wrote:
> >
> > On Wed, Jun 22, 2022 at 11:09 AM Masahiko Sawada  
> > wrote:
> > >
> > > I've attached a WIP patch for adding regression tests for DDL deparse.
> > > The patch can be applied on
> > > v9-0001-Functions-to-deparse-DDL-commands.patch Hou recently
> > > submitted[1]. The basic idea is to define the event trigger to deparse
> > > DDLs, run the regression tests, load the deparsed DDLs to another
> > > database cluster, dump both databases, and compare the dumps.
> > >
> >
> > Thanks for working on this. It is a good start. I think this will be
> > helpful to see the missing DDL support. Do we want to run this as part
> > of every regression run? Won't it increase the regression time as this
> > seems to run internally the regression tests twice?
>
> Yes, It will increase the regression test time but we already do a
> similar thing in 002_pg_upgrade.pl and 027_stream_regress.pl and it
> seems to be worth adding to me.
>

Fair enough. I think here we need to run it twice once before deparse
and once after deparsing whereas those tests seem to be running it one
time. That might not matter much but we can check the timing
difference.  I agree that we anyway need something like this to verify
the deparsing code.

> >
> > Do we need a different trigger to capture drop cases as there are
> > separate deparsing routines for them, for example
> > deparse_drop_table()?
>
> Right, we need to capture drop cases by another trigger.
>

And probably something for alter subcommands as well.

-- 
With Regards,
Amit Kapila.

Re: allow specifying action when standby encounters incompatible parameter settings

[Simon Riggs]

On Thu, 23 Jun 2022 at 18:45, Nathan Bossart  wrote:
>
> Thanks for chiming in.
>
> On Thu, Jun 23, 2022 at 04:19:45PM +0100, Simon Riggs wrote:
> > I don't understand why you need this patch at all.
> >
> > Since you have automation, you can use that layer to automatically
> > restart all standbys at once, if you choose, without any help or
> > hindrance from PostgreSQL.
> >
> > But I really don't want you to do that, since that results in loss of
> > availability of the service. I'd like you to try a little harder and
> > automate this in a way that allows the service to continue with some
> > standbys available while others restart.
> >
> > All this feature does is allow you to implement things in a lazy way
> > that causes a loss of availability for users. I don't think that is of
> > benefit to PostgreSQL users, so -1 from me, on this patch (only),
> > sorry about that.
>
> Overall, this is intended for users that care more about keeping WAL replay
> caught up than a temporary loss of availability due to a restart.  Without
> this, I'd need to detect that WAL replay has paused due to insufficient
> parameters and restart Postgres. If І can configure Postgres to
> automatically shut down in these scenarios, my automation can skip right to
> adjusting the parameters and starting Postgres up.  Of course, if you care
> more about availability, you'd keep this parameter set to the default
> (pause) and restart on your own schedule.

There are a few choices of how we can deal with this situation
1. Make the change blindly and then pick up the pieces afterwards
2. Check the configuration before changes are made, and make the
changes in the right order

This patch and the above argument assumes that you must do (1), but
you could easily do (2).

i.e. If you know that changing specific parameters might affect
availability, why not query those parameter values on all servers
first and check whether the change will affect availability, before
you allow the changes? why rely on PostgreSQL to pick up the pieces
because the orchestration code doesn't (yet) make configuration sanity
checks?

This patch would undo a very important change - to keep servers
available by default and go back to the old behavior for a huge fleet
of Postgres databases. The old behavior of shutdown-on-change caused
catastrophe many times for users and in one case brought down a rather
large and important service provider, whose CTO explained to me quite
clearly how stupid he thought that old behavior was. So I will not
easily agree to allowing it to be put back into PostgreSQL, simply to
avoid adding a small amount of easy code into an orchestration layer
that could and should implement documented best practice.

I am otherwise very appreciative of your insightful contributions,
just not this specific one.

Happy to discuss how we might introduce new parameters/behavior to
reduce the list of parameters that need to be kept in lock-step.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: O(n) tasks cause lengthy startups and checkpoints

[Simon Riggs]

On Thu, 23 Jun 2022 at 18:15, Nathan Bossart  wrote:

> I'm grateful for the discussion in this thread so far, but I'm not seeing a
> clear path forward.

+1 to add the new auxiliary process.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: array_cat anycompatible change is breaking xversion upgrade tests

[Andrey Borodin]

Hi everyone!

Sorry for bumping old thread.

> On 25 May 2021, at 21:14, Justin Pryzby  wrote:
> 
> Such aggregate functions should be dropped before upgrade/restore and then
> re-created afterwards using the "anycompatible" functions.  The affected
> functions are: array_append, array_prepend, array_cat, array_position,
> array_positions, array_remove, array_replace, and width_bucket.

We've just stumbled upon the problem in our service. Would it be backpatchable 
to add this check to pg_upgrade?

I want to check something like

select * from pg_aggregate join pg_proc on (aggtransfn = pg_proc.oid)
where proname in ('array_append', 'array_prepend','array_cat', 
'array_position','array_positions', 'array_remove', 'array_replace', 
'width_bucket') ;

select * from pg_operator join pg_proc on (oprcode = pg_proc.oid)
where proname in ('array_append', 'array_prepend','array_cat', 
'array_position','array_positions', 'array_remove', 'array_replace', 
'width_bucket') and pg_operator.oid >= 16384;

if pg_upgrade is executed with --check option.

Best regards, Andrey Borodin.

Re: Unify DLSUFFIX on Darwin

[Peter Eisentraut]

On 22.06.22 15:45, Tom Lane wrote:

Peter Eisentraut  writes:

macOS has traditionally used extension .dylib for shared libraries (used
at build time) and .so for dynamically loaded modules (used by
dlopen()).  This complicates the build system a bit.  Also, Meson uses
.dylib for both, so it would be worth unifying this in order to be able
to get equal build output.



There doesn't appear to be any reason to use any particular extension
for dlopened modules, since dlopen() will accept anything and PostgreSQL
is well-factored to be able to deal with any extension.  Other software
packages that I have handy appear to be about 50/50 split on which
extension they use for their plugins.  So it seems possible to change
this safely.


Doesn't this amount to a fundamental ABI break for extensions?
Yesterday they had to ship foo.so, today they have to ship foo.dylib.


Extensions generally only load the module files using the extension-free 
base name.  And if they do specify the extension, they should use the 
provided DLSUFFIX variable and not hardcode it.  So I don't see how this 
would be a problem.

Re: CREATE TABLE ( .. STORAGE ..)

[Aleksander Alekseev]

Hi hackers,

Many thanks for the review!

Here is a patch updated according to all the recent feedback, except
for two suggestions:

> This adds support for "ADD COLUMN SET STORAGE" but it is not described
> in the doc.  COMPRESSION is not described, too.  Shouldn't we add the
> both this time?  Or the fix for COMPRESSION can be a different patch.

The documentation for ADD COLUMN simply says:

```
 
 This form adds a new column to the table, using the same syntax as
 CREATE
TABLE. If IF NOT EXISTS
 is specified and a column already exists with this name,
 no error is thrown.
 
```

I suggest keeping a reference to CREATE TABLE, similarly as it was
done for ALTER COLUMN.

> Now that we have three column options COMPRESSION, COLLATE and STORGE
> which has the strict order in syntax.  I wonder it can be relaxed but
> it might be too much..

Agree, this could be a bit too much for this particular discussion.
Although this shouldn't be a difficult change, and I agree that this
should be useful, personally I don't feel enthusiastic enough to
deliver it right now. I suggest we address this later.

-- 
Best regards,
Aleksander Alekseev


v4-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch
Description: Binary data

Re: fix crash with Python 3.11

[Markus Wanner]

On 6/24/22 00:54, Tom Lane wrote:

Does such code exist?  I don't see any other calls in Debian code search,
and I find it hard to believe that anyone would think such a thing is
maintainable.


Such a thing does exist within PGLogical and BDR, yes.

Thanks for your concern about maintainability. So far, that part was not 
posing any trouble. Looking at e.g. postgres.c, the sigsetjmp handler 
there didn't change all that much in recent years. Much of the code 
there is from around 2004 written by you.


However, that shouldn't be your concern at all. Postgres refusing to 
start after a minor upgrade probably should, especially when it's due to 
an API change in a stable branch.


Regards

Markus

Re: NAMEDATALEN increase because of non-latin languages

[Robert Haas]

On Thu, Jun 23, 2022 at 6:43 PM Tom Lane  wrote:
> Nonetheless, the presence of GETSTRUCT calls should be a good guide
> to where we need to do something.

Indubitably.

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

Re: NAMEDATALEN increase because of non-latin languages

[Robert Haas]

On Thu, Jun 23, 2022 at 11:11 PM John Naylor
 wrote:
> Hmm, I must have misunderstood this aspect. In my mind I was thinking
> that if a varlen attribute were at the end, these functions would make
> it easier to access them quickly. But from this and the follow-on
> responses, these would be used to access varlen attributes wherever
> they may be. I'll take a look at the current uses of GETSTRUCT().

I don't know whether we can or should move all the "name" columns to
the end of the catalog. It would be user-visible and probably not
user-desirable, but it would save something in terms of tuple
deforming cost. I'm just not sure how much.

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

pg_receivewal unable to connect to promoted standby

[RKN Sai Krishna]

Hi,

I'm trying to have a setup where there is a primary, standby and
pg_receivewal (which acts as a server that maintains the entire WAL).
Quorum is any one of standby and pg_receivewal. In case of primary crash,
when I promote standby (timeline switch from 5 to 6) and restart
pg_receivewal to connect to the promoted standby, I get an error saying
"pg_receivewal: could not send replication command "START_REPLICATION":
ERROR:  requested starting point 16/4C00 on timeline 5 is not in this
server's history. This server's history forked from timeline 5 at
16/4BFFF268".

pg_receivewal latest lsn is 16/4BFFF268 with the timeline id being 5.

Just wondering why is the pg_receivewal requesting the new primary with the
starting point as 16/4C00, even though the latest lsn is 16/4BFFF268.

Is that because of the following code snippet in pg_receivewal by any
chance?

/*
* Move the starting pointer to the start of the next segment, if the
* highest one we saw was completed. Otherwise start streaming from
* the beginning of the .partial segment.
*/
if (!high_ispartial)
high_segno++;

If it is because of the above code, Can we let the pg_receivewal request
the new primary to provide WAL from forked lsn (by asking primary what the
forked lsn and the corresponding timeline are)?

Thanks,
RKN

Re: CREATE TABLE ( .. STORAGE ..)

[Aleksander Alekseev]

Hi hackers,

> Here is a patch updated according to all the recent feedback, except
> for two suggestions:

In v4 I forgot to list possible arguments for STORAGE in
alter_table.sgml, similarly as it is done for other subcommands. Here
is a corrected patch.

- SET STORAGE
+ SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }

-- 
Best regards,
Aleksander Alekseev


v5-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch
Description: Binary data

Re: array_cat anycompatible change is breaking xversion upgrade tests

[Andrey Borodin]

> On 24 Jun 2022, at 16:09, Andrey Borodin  wrote:
> 
> Would it be backpatchable to add this check to pg_upgrade?

Just to be clear of what exactly I propose I drafted a patch. PFA.
I've tested it with PG13 and
CREATE AGGREGATE public.array_accum(anyelement) (
 SFUNC = array_append,
 STYPE = anyarray,
 INITCOND = '{}',
 PARALLEL = safe
);
CREATE OPERATOR ##% (leftarg=anyarray, 
rightarg=anyelement,function=array_append);

Operator output currently look a bit strage, but does it's job. 
pg_upgrade_output.d/operators.txt:
In database: postgres
  (oid=16385) ##% in public

Thanks!

Best regards, Andrey Borodin.


v1-0001-Check-incompatible-aggreagtes-and-operators-befor.patch
Description: Binary data

Re: array_cat anycompatible change is breaking xversion upgrade tests

[Justin Pryzby]

On Fri, Jun 24, 2022 at 04:09:46PM +0500, Andrey Borodin wrote:
> Hi everyone!
> 
> Sorry for bumping old thread.

Please find this newer thread+patch here ;)
https://www.postgresql.org/message-id/20220614230949.gx29...@telsasoft.com

> On 25 May 2021, at 21:14, Justin Pryzby  wrote:
> > 
> > Such aggregate functions should be dropped before upgrade/restore and then
> > re-created afterwards using the "anycompatible" functions.  The affected
> > functions are: array_append, array_prepend, array_cat, array_position,
> > array_positions, array_remove, array_replace, and width_bucket.
> 
> We've just stumbled upon the problem in our service. Would it be 
> backpatchable to add this check to pg_upgrade?

I guess you mean to backpatch to v14 for people upgrading from v13.

I realized that my latest patch would break upgrades from old servers, which do
not have array_position/s nor width_bucket, so ::reprocedure would fail.  Maybe
Andrey's way is better (checking proname rather than its OID).

-- 
Justin

Re: array_cat anycompatible change is breaking xversion upgrade tests

[Tom Lane]

Justin Pryzby  writes:
> I realized that my latest patch would break upgrades from old servers, which 
> do
> not have array_position/s nor width_bucket, so ::reprocedure would fail.  
> Maybe
> Andrey's way is better (checking proname rather than its OID).

proname is dangerous, because there's nothing stopping users from
adding more functions with the same name.

Just use a server-version-dependent list of regprocedure OIDs.

regards, tom lane

Re: NAMEDATALEN increase because of non-latin languages

[Tom Lane]

Robert Haas  writes:
> I don't know whether we can or should move all the "name" columns to
> the end of the catalog. It would be user-visible and probably not
> user-desirable,

I'm a strong -1 on changing that if we're not absolutely forced to.

> but it would save something in terms of tuple
> deforming cost. I'm just not sure how much.

I'd guess nothing, if we are deforming all the fields.

regards, tom lane

Re: Unify DLSUFFIX on Darwin

[Tom Lane]

Peter Eisentraut  writes:
> On 22.06.22 15:45, Tom Lane wrote:
>> Doesn't this amount to a fundamental ABI break for extensions?
>> Yesterday they had to ship foo.so, today they have to ship foo.dylib.

> Extensions generally only load the module files using the extension-free 
> base name.  And if they do specify the extension, they should use the 
> provided DLSUFFIX variable and not hardcode it.  So I don't see how this 
> would be a problem.

Hm.  Since we force people to recompile extensions for new major versions
anyway, maybe it'd be all right.  I'm sure there is *somebody* out there
who will have to adjust their build scripts, but it does seem like it
shouldn't be much worse than other routine API changes.

[ thinks for a bit... ]  Might be worth double-checking that pg_upgrade
doesn't get confused in a cross-version upgrade.  A quick grep doesn't
find that it refers to DLSUFFIX anywhere, but it definitely does pay
attention to extensions' shared library names.

regards, tom lane

Re: Make COPY extendable in order to support Parquet and other formats

[Andrew Dunstan]

On 2022-06-23 Th 21:45, Andres Freund wrote:
> Hi,
>
> On 2022-06-23 11:38:29 +0300, Aleksander Alekseev wrote:
>>> I know little about parquet - can it support FROM STDIN efficiently?
>> Parquet is a compressed binary format with data grouped by columns
>> [1]. I wouldn't assume that this is a primary use case for this
>> particular format.
> IMO decent COPY FROM / TO STDIN support is crucial, because otherwise you
> can't do COPY from/to a client. Which would make the feature unusable for
> anybody not superuser, including just about all users of hosted PG.
>

+1


Note that Parquet puts the metadata at the end of each file, which makes
it nice to write but somewhat unfriendly for streaming readers, which
would have to accumulate the whole file in order to process it.


cheers


andrew


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

Implement hook for self-join simplification

[Leif Harald Karlsen]

Hi,


I have a made a small deductive database on top of PostgreSQL for 
educational/research purposes. In this setting, due to certain 
VIEW-constructions, queries often end up being self-joins on primary keys, e.g.:


SELECT t1.id, t2.val

FROM t AS t1 JOIN t AS t2 USING (id);


where t(id) is a primary key. This query is equivalent to the much more 
efficient:


SELECT id, val

FROM t AS t1;


However, PostgreSQL currently does not seem to implement this simplification. 
Therefore, I have looked into writing an extension that performs this, but I am 
struggling a bit with finding out when this simplification should be done, i.e. 
which hook I should implement.


The simplification is not too different from those done in prep/prepjoin.c, 
i.e. doing the simplification on the query-tree directly. However, I think I 
then would need to implement a planner_hook, as it is the only hook giving me 
direct access to the query-tree. But I need to perform my simplification after 
view-definitions have been expanded into the query, and after the 
transformations in prepjoin.c (but before the rest of planning). But there 
seems to be no easy way to inject a function there, as this is buried deep in 
the middle of the planner-function.


I therefore looked into using a set_join_pathlist_hook, and try to do the 
simplification at path-level. I.e., doing something like:


static void self_join_optimize_hook(PlannerInfo *root, RelOptInfo* joinrel, 
RelOptInfo* outerrel, RelOptInfo* innerrel, JoinType jointype, 
JoinPathExtraData* extra)

{

if (is_selfjoin_on_pk(root, joinrel, extra)) {

ListCell *p;

foreach(p, innerrel->pathlist) {

add_path(joinrel, (Path *) p);

}

}

}


That is, if joinrel is a (binary) self-join on a primary key, the paths for 
evaluating the join is the same as the paths for evaluating the innerrel, 
However, this does not work, as the rest of the query may require values from 
the other table (e.g. t2 in the example above). I therefore need to replace all 
mentions of t2 with t1, but is this possible at a path-level?


If not, does anyone have a an idea on how this can be done in a different way? 
Thanks!



Kind regards,


Leif Harald Karlsen

Senior Lecturer

Department of Informatics

University of Oslo

Re: WIP Patch: Add a function that returns binary JSONB as a bytea

[Jelte Fennema]

> It's a decent amount of work to define one though... It's clearly not
> acceptable to just dump out the internal representation, as already discussed
> in this thread.

I totally agree that it should be a well-defined format that doesn't
leak stuff like endianness and alignment of the underlying database.

With a bit of googling I found the UBJSON specification:
https://ubjson.org/#data_format
It seems like it would be possible to transform between JSONB and
UBJSON efficiently. As an example: For my recent use case the main
thing that was slow was parsing JSON strings, because of the escape
characters. That's not needed with UBJSON, because strings are simply
UTF-8 encoded binary data, that are prefixed with their length. So all
that would be needed is checking if the binary data is valid UTF-8.

Also there seem to be implementations in many languages for this spec:
https://ubjson.org/libraries/ So, that should make it easy for
Postgres client libraries to support this binary format.

> I'm still bemused by the proposition that that common interchange format
> shouldn't be, um, JSON. We've already seen BSON, BJSON, etc die
> well-deserved deaths.

@Tom Lane: UBJSON calls explicitly lists these specific failed
attempts at a binary encoding for JSON as the reason for why it was
created, aiming to fix the issues those specs have:
https://github.com/ubjson/universal-binary-json#why

Jelte

Pre-installed index access methods cannot be manually installed.

[Matthias van de Meent]

Hi,

I noticed that in many places we check or assert the OIDs of our
built-in AMs. E.g. in planning, we will only do row compares in
indexes that have BTREE_AM_OID, and we can only sort tuples for
btamhandler -based index ams if their oid is the BTREE_AM_OID. That
seems like an artificial limitation to me.

Although it makes sense to ensure that we don't accidentally call such
functions from the 'wrong location', it does mean that a user cannot
manually install the preinstalled access methods and get a working
index AM, because the internal code is checking the OID of the
supplied relation's access method, which will not match the expected
value when manually installed.

Is this expected? Would we accept patches that remove or reduce the
impact of these artificial limitations?

Kind regards,

Matthias van de Meent

PS. I noticed this when checking the sortsupport code for some active
patches, and after playing around a bit while trying to run PG
extensions that are not system-registered. The attached test case
fails, where I'd expect it to succeed, or at least I expected it not
to fail at "This AM's OID has an unexpected value".


scratch_33.sql
Description: Binary data

Re: Pre-installed index access methods cannot be manually installed.

[Matthias van de Meent]

> PS. I noticed this when checking the sortsupport code for some active
> patches, and after playing around a bit while trying to run PG
> extensions that are not system-registered. The attached test case
> fails, where I'd expect it to succeed, or at least I expected it not
> to fail at "This AM's OID has an unexpected value".

I just realised that the failure of the specific mentioned test case
was unrelated to the issue at hand, as it correctly shows that you
can't use int8-opclasses for int4 columns.

The attached fixes that test case by creating a table with a bigint
column instead, so that the test correctly, but unexpectedly, outputs
"ERROR:  unexpected non-btree AM: N".

- Matthias


scratch_33.sql
Description: Binary data

Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

[Andrew Dunstan]

On 2022-06-23 Th 21:51, Andres Freund wrote:
> Hi,
>
> On 2022-06-23 16:38:12 +0900, Michael Paquier wrote:
>> On Tue, Jun 21, 2022 at 05:41:07PM -0400, Andrew Dunstan wrote:
>>> On 2022-06-21 Tu 17:25, Andres Freund wrote:
 On 2022-06-21 17:11:33 -0400, Andrew Dunstan wrote:
> I and a couple of colleagues have looked it over. As far as it goes the
> json fix looks kosher to me. I'll play with it some more.
 Cool.

 Any chance you could look at fixing the "structure" of the generated
 expression "program". The recursive ExecEvalExpr() calls are really not 
 ok...
>> By how much does the size of ExprEvalStep go down once you don't
>> inline the JSON structures as of 0004 in [1]?  And what of 0003?
> 0004 gets us back to 64 bytes, if 0003 is applied first. 0003 alone doesn't
> yield a size reduction, because obviously 0004 is the bigger problem. Applying
> just 0004 you end up with 88 bytes.
>
>
>> The JSON portions seem like the largest portion of the cake, though both are
>> must-fixes.
> Yep.
>
>
>>> Yes, but I don't guarantee to have a fix in time for Beta2.
>> IMHO, it would be nice to get something done for beta2.  Now the
>> thread is rather fresh and I guess that more performance study is
>> required even for 0004, so..
> I don't think there's a whole lot of performance study needed for 0004 - the
> current state is obviously wrong.
>
> I think Andrew's beta 2 comment was more about my other architectural
> complains around the json expression eval stuff.


Right. That's being worked on but it's not going to be a mechanical fix.


>
>
>> Waiting for beta3 would a better move at this stage.  Is somebody confident
>> enough in the patches proposed?
> 0001 is the one that needs to most careful analysis, I think. 0002 I'd be fine
> with pushing after reviewing it again. For 0003 David's approach might be
> better or worse, it doesn't matter much I think. 0004 is ok I think, perhaps
> with the exception of quibbling over some naming decisions?
>
>

The attached very small patch applies on top of your 0002 and deals with
the FmgrInfo complaint.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From ed2118c63e298e5e5872db5003fa342c33aa7505 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Wed, 22 Jun 2022 12:00:47 -0400
Subject: [PATCH 2/2] in JsonExprState just store a pointer to the input
 FmgrInfo

---
 src/backend/executor/execExpr.c   | 5 -
 src/backend/executor/execExprInterp.c | 2 +-
 src/include/executor/execExpr.h   | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 1f65daf203..d8608a3c58 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2623,11 +2623,14 @@ ExecInitExprRec(Expr *node, ExprState *state,
 	(jexpr->result_coercion && jexpr->result_coercion->via_io))
 {
 	Oid			typinput;
+	FmgrInfo   *finfo;
 
 	/* lookup the result type's input function */
 	getTypeInputInfo(jexpr->returning->typid, &typinput,
 	 &jsestate->input.typioparam);
-	fmgr_info(typinput, &jsestate->input.func);
+	finfo = palloc0(sizeof(FmgrInfo));
+	fmgr_info(typinput, finfo);
+	jsestate->input.finfo = finfo;
 }
 
 jsestate->args = NIL;
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 80c5e04dd5..cbdcbbce53 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4648,7 +4648,7 @@ ExecEvalJsonExprCoercion(ExprEvalStep *op, ExprContext *econtext,
 			/* strip quotes and call typinput function */
 			char	   *str = *isNull ? NULL : JsonbUnquote(jb);
 
-			return InputFunctionCall(&jsestate->input.func, str,
+			return InputFunctionCall(jsestate->input.finfo, str,
 	 jsestate->input.typioparam,
 	 jexpr->returning->typmod);
 		}
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 15532813ae..c0ba8998f2 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -759,7 +759,7 @@ typedef struct JsonExprState
 
 	struct
 	{
-		FmgrInfo	func;	/* typinput function for output type */
+		FmgrInfo	*finfo;	/* typinput function for output type */
 		Oid			typioparam;
 	}			input;	/* I/O info for output type */
 
-- 
2.34.1

Re: Pre-installed index access methods cannot be manually installed.

[Tom Lane]

Matthias van de Meent  writes:
> Although it makes sense to ensure that we don't accidentally call such
> functions from the 'wrong location', it does mean that a user cannot
> manually install the preinstalled access methods and get a working
> index AM, because the internal code is checking the OID of the
> supplied relation's access method, which will not match the expected
> value when manually installed.

This seems like a straw argument, considering that there would be
dozens of other problems in the way of removing or replacing any
built-in index AMs.

> Is this expected? Would we accept patches that remove or reduce the
> impact of these artificial limitations?

Seems like a significant waste of effort to me.

regards, tom lane

Re: making relfilenodes 56 bits

[Robert Haas]

On Fri, Jun 24, 2022 at 7:08 AM Dilip Kumar  wrote:
> I have changed that. PFA, the updated patch.

Apart from one minor nitpick (see below) I don't see a problem with
this in isolation. It seems like a pretty clean renaming. So I think
we need to move onto the question of how clean the rest of the patch
series looks with this as a base.

A preliminary refactoring that was discussed in the past and was
originally in 0001 was to move the fields included in BufferTag via
RelFileNode/Locator directly into the struct. I think maybe it doesn't
make sense to include that in 0001 as you have it here, but maybe that
could be 0002 with the main patch to follow as 0003, or something like
that. I wonder if we can get by with redefining RelFileNode like this
in 0002:

typedef struct buftag
{
Oid spcOid;
Oid dbOid;
RelFileNumber   fileNumber;
ForkNumber  forkNum;
} BufferTag;

And then like this in 0003:

typedef struct buftag
{
Oid spcOid;
Oid dbOid;
RelFileNumber   fileNumber:56;
ForkNumber  forkNum:8;
} BufferTag;

- * from catalog OIDs to filenode numbers.  Each database has a map file for
+ * from catalog OIDs to filenumber.  Each database has a map file for

should be filenumbers

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

Re: Make COPY extendable in order to support Parquet and other formats

[Aleksander Alekseev]

Hi Andrew,

> > IMO decent COPY FROM / TO STDIN support is crucial, because otherwise you
> > can't do COPY from/to a client. Which would make the feature unusable for
> > anybody not superuser, including just about all users of hosted PG.
> >
>
> +1
>
> Note that Parquet puts the metadata at the end of each file, which makes
> it nice to write but somewhat unfriendly for streaming readers, which
> would have to accumulate the whole file in order to process it.

It's not necessarily that bad since data is divided into pages, each
page can be processed separately. However personally I have limited
experience with Parquet at this point. Some experimentation is
required. I will keep in mind the requirement regarding COPY FROM / TO
STDIN.

-- 
Best regards,
Aleksander Alekseev

Re: [BUG] Panic due to incorrect missingContrecPtr after promotion

[Imseih (AWS), Sami]

>Thus, I still don't see what have happened at Imseih's hand, but I can
>cause PANIC with a bit tricky steps, which I don't think valid.  This
>is what I wanted to know the exact steps to cause the PANIC.

>The attached 1 is the PoC of the TAP test (it uses system()..), and
>the second is a tentative fix for that.  (I don't like the fix, too,
>though...)

It is been difficult to get a generic repro, but the way we reproduce
Is through our test suite. To give more details, we are running tests
In which we constantly failover and promote standbys. The issue
surfaces after we have gone through a few promotions which occur
every few hours or so ( not really important but to give context ).

I am adding some additional debugging  to see if I can draw a better
picture of what is happening. Will also give aborted_contrec_reset_3.patch 
a go, although I suspect it will not handle the specific case we are deaing 
with.


Regards,

Sami imseih
Amazon Web Services

Re: WIP Patch: Add a function that returns binary JSONB as a bytea

[Andres Freund]

Hi,

On 2022-06-24 14:33:00 +0700, John Naylor wrote:
> On Thu, Jun 23, 2022 at 9:06 PM Andres Freund  wrote:
> > It looks like there's quite a bit of low hanging fruits to optimize...
> 
> Yeah, if escapes and control characters are rare, adding an SSE2 fast
> path would give a boost to json_lex_string: check 16 bytes at a time
> for those chars (plus the ending double-quote).

The biggest thing I see is building the string in bigger chunks. Doing a
separate appendStringInfoChar() for each input character is obviously bad for
performance. I'd bet a good chunk of the time attributed to json_lex_string()
in Jelte's flamegraph is actually setting up the external function call to
appendStringInfoChar(). Which then proceeds to do a check whether the buffer
needs to be enlarged and maintains the trailing null byte on every call.

Greetings,

Andres Freund

Re: [PATCH] Add extra statistics to explain for Nested Loop

[Ekaterina Sokolova]

Hi, hackers!

We started discussion about overheads and how to calculate it correctly.

Julien Rouhaud wrote:
Can you give a bit more details on your bench scenario?  I see 
contradictory
results, where the patched version with more code is sometimes way 
faster,

sometimes way slower.  If you're using pgbench
default queries (including write queries) I don't think that any of 
them will
hit the loop code, so it's really a best case scenario.  Also write 
queries

will make tests less stable for no added value wrt. this code.

Ideally you would need a custom scenario with a single read-only query
involving a nested loop or something like that to check how much 
overhead you

really get when you cumulate those values.
I created 2 custom scenarios. First one contains VERBOSE flag so this 
scenario uses extra statistics. Second one doesn't use new feature and 
doesn't disable its use (therefore still collect data).

I attach scripts for pgbench to this letter.

Main conclusions are:
1) the use of additional statistics affects no more than 4.5%;
2) data collection affects no more than 1.5%.
I think testing on another machine would be very helpful, so if you get 
a chance, I'd be happy if you share your observations.


Some fixes:

Sure, but if we're going to have a branch for nloops == 0, I think it 
would be

better to avoid redundant / useless instructions

Right. I done it.

Justin Pryzby wrote:

Maybe set parallel_leader_participation=no for this test.
Thanks for reporting the issue and advice. I set 
parallel_leader_participation = off. I hope this helps to solve the 
problem of inconsistencies in the outputs.


If you have any comments on this topic or want to share your 
impressions, please write to me.
Thank you very much for your contribution to the development of this 
patch.


--
Ekaterina Sokolova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres CompanyFrom: Justin Pryzby 
Subject: [PATCH 1/2] explain.c-refactor-ExplainNode_v3

---
 src/backend/commands/explain.c | 110 ++---
 1 file changed, 48 insertions(+), 62 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5d1f7089daf..bd785c6741d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -118,6 +118,8 @@ static void show_instrumentation_count(const char *qlabel, int which,
 	   PlanState *planstate, ExplainState *es);
 static void show_foreignscan_info(ForeignScanState *fsstate, ExplainState *es);
 static void show_eval_params(Bitmapset *bms_params, ExplainState *es);
+static void show_loop_info(Instrumentation *instrument, bool isworker,
+		   ExplainState *es);
 static const char *explain_get_index_name(Oid indexId);
 static void show_buffer_usage(ExplainState *es, const BufferUsage *usage,
 			  bool planning);
@@ -1615,36 +1617,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
 
 	if (es->analyze &&
 		planstate->instrument && planstate->instrument->nloops > 0)
-	{
-		double		nloops = planstate->instrument->nloops;
-		double		startup_ms = 1000.0 * planstate->instrument->startup / nloops;
-		double		total_ms = 1000.0 * planstate->instrument->total / nloops;
-		double		rows = planstate->instrument->ntuples / nloops;
-
-		if (es->format == EXPLAIN_FORMAT_TEXT)
-		{
-			if (es->timing)
-appendStringInfo(es->str,
- " (actual time=%.3f..%.3f rows=%.0f loops=%.0f)",
- startup_ms, total_ms, rows, nloops);
-			else
-appendStringInfo(es->str,
- " (actual rows=%.0f loops=%.0f)",
- rows, nloops);
-		}
-		else
-		{
-			if (es->timing)
-			{
-ExplainPropertyFloat("Actual Startup Time", "ms", startup_ms,
-	 3, es);
-ExplainPropertyFloat("Actual Total Time", "ms", total_ms,
-	 3, es);
-			}
-			ExplainPropertyFloat("Actual Rows", NULL, rows, 0, es);
-			ExplainPropertyFloat("Actual Loops", NULL, nloops, 0, es);
-		}
-	}
+		show_loop_info(planstate->instrument, false, es);
 	else if (es->analyze)
 	{
 		if (es->format == EXPLAIN_FORMAT_TEXT)
@@ -1673,44 +1646,16 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		for (int n = 0; n < w->num_workers; n++)
 		{
 			Instrumentation *instrument = &w->instrument[n];
-			double		nloops = instrument->nloops;
-			double		startup_ms;
-			double		total_ms;
-			double		rows;
 
-			if (nloops <= 0)
+			if (instrument->nloops <= 0)
 continue;
-			startup_ms = 1000.0 * instrument->startup / nloops;
-			total_ms = 1000.0 * instrument->total / nloops;
-			rows = instrument->ntuples / nloops;
 
 			ExplainOpenWorker(n, es);
 
-			if (es->format == EXPLAIN_FORMAT_TEXT)
-			{
-ExplainIndentText(es);
-if (es->timing)
-	appendStringInfo(es->str,
-	 "actual time=%.3f..%.3f rows=%.0f loops=%.0f\n",
-	 startup_ms, total_ms, rows, nloops);
-else
-	appendStringInfo(es->str,
-	 "actual rows=%.0f loops=%.0f\n",
-	 rows, nloops);
-			}
-			else
-			{
-if (es->timing

Re: Implement hook for self-join simplification

[Andrey Lepikhov]

On 24/6/2022 18:58, Leif Harald Karlsen wrote:
I have a made a small deductive database on top of PostgreSQL for 
educational/research purposes. In this setting, due to certain 
VIEW-constructions, queries often end up being self-joins on primary 
keys, e.g.:

SELECT t1.id, t2.val
FROM t AS t1 JOIN t AS t2 USING (id);

where t(id) is a primary key. This query is equivalent to the much more 
efficient:

SELECT id, val FROM t AS t1;

However, PostgreSQL currently does not seem to implement this 
simplification. Therefore, I have looked into writing an extension that 
performs this, but I am struggling a bit with finding out when this 
simplification should be done, i.e. which hook I should implement.
It is true, but you can use a proposed patch that adds such 
functionality [1].


I tried to reproduce your case:
CREATE TABLE t(id int PRIMARY KEY, val text);
explain verbose
SELECT t1.id, t2.val FROM t AS t1 JOIN t AS t2 USING (id);

With this patch you will get a plan:
 Seq Scan on public.t t2
   Output: t2.id, t2.val
   Filter: (t2.id IS NOT NULL)

The approach, implemented in this patch looks better because removes 
self-joins on earlier stage than the path generation stage. Feel free to 
use it in your research.


[1] 
https://www.postgresql.org/message-id/a1d6290c-44e0-0dfc-3fca-66a68b310...@postgrespro.ru


--
regards,
Andrey Lepikhov
Postgres Professional

Re: array_cat anycompatible change is breaking xversion upgrade tests

[Andrey Borodin]

> On 24 Jun 2022, at 18:30, Justin Pryzby  wrote:
> 
> On Fri, Jun 24, 2022 at 04:09:46PM +0500, Andrey Borodin wrote:
>> Hi everyone!
>> 
>> Sorry for bumping old thread.
> 
> Please find this newer thread+patch here ;)
> https://www.postgresql.org/message-id/20220614230949.gx29...@telsasoft.com

Oops. Let's discard my patch and I'll review yours. Many thanks for fixing this 
stuff :)

> On 24 Jun 2022, at 19:06, Tom Lane  wrote:
> 
> Justin Pryzby  writes:
>> I realized that my latest patch would break upgrades from old servers, which 
>> do
>> not have array_position/s nor width_bucket, so ::reprocedure would fail.  
>> Maybe
>> Andrey's way is better (checking proname rather than its OID).
> 
> proname is dangerous, because there's nothing stopping users from
> adding more functions with the same name.
> 
> Just use a server-version-dependent list of regprocedure OIDs.

Server-version-dependent list of oids seems more error prone. I think we can 
just check proname by the list and oid < 16384.

Thanks!

Best regards, Andrey Borodin.

Re: pg_upgrade (12->14) fails on aggregate

[Andrey Borodin]

> On 23 Jun 2022, at 04:58, Justin Pryzby  wrote:
> 
> On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
>> Robert Haas  writes:
>>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby  wrote:
 To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
>> 
>>> Extensions can be installed into pg_catalog, but they can't get
>>> low-numbered OIDs.
>> 
>> Exactly.  (To be clear, I had in mind writing something involving
>> FirstNormalObjectId, not that you should put literal "16384" in the
>> code.)
> 
> Actually, 16384 is already used in two other places in check.c, so ...

Yes, but it's a third copy of the comment ("* The query below hardcodes 
FirstNormalObjectId as 16384 rather than") across the file.

Also, we can return slightly more information about found objects. For example, 
operator will look like "operator: ||". At least we can get nspname and oid. 
And, maybe return type for aggregator and leftarg\rightarg types for operator?

BTW comment /* Before v11, used proisagg=true, and afterwards uses prokind='a' 
*/ seems interesting, but irrelevant. We join with pg_aggregate anyway.

Thanks!

Best regards, Andrey Borodin.

Re: [PoC] Let libpq reject unexpected authentication requests

[Jacob Champion]

On Thu, Jun 23, 2022 at 10:33 AM Jacob Champion  wrote:
> - I think NOT is a important case in practice, which is effectively a
> negative OR ("anything but this/these")

Both NOT (via ! negation) and "none" are implemented in v4.

Examples:

# The server must use SCRAM.
require_auth=scram-sha-256
# The server must use SCRAM or Kerberos.
require_auth=scram-sha-256,gss,sspi
# The server may optionally use SCRAM.
require_auth=none,scram-sha-256
# The server must not use any application-level authentication.
require_auth=none
# The server may optionally use authentication, except plaintext
# passwords.
require_auth=!password
# The server may optionally use authentication, except weaker password
# challenges.
require_auth=!password,!md5
# The server must use an authentication method.
require_auth=!none
# The server must use a non-plaintext authentication method.
require_auth=!none,!password

Note that `require_auth=none,scram-sha-256` allows the server to
abandon a SCRAM exchange early, same as it can today. That might be a
bit surprising.

--Jacob
commit dcae9a7c1cef593055c932c85244852a119b0313
Author: Jacob Champion 
Date:   Thu Jun 23 15:38:12 2022 -0700

squash! libpq: let client reject unexpected auth methods

- Add "none" and method negation with "!".
- Test unknown methods to round out coverage.

TODO: should "none,scram-sha-256" allow an incomplete SCRAM handshake?

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 00990ce47d..cc831158b7 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1233,6 +1233,20 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 for the connection to succeed. By default, any authentication method is
 accepted, and the server is free to skip authentication altogether.
   
+  
+Methods may be negated with the addition of a !
+prefix, in which case the server must not attempt
+the listed method; any other method is accepted, and the server is free
+not to authenticate the client at all. If a comma-separated list is
+provided, the server may not attempt any of the
+listed negated methods. Negated and non-negated forms may not be
+combined in the same setting.
+  
+  
+As a final special case, the none method requires 
the
+server not to use an authentication challenge. (It may also be negated,
+to require some form of authentication.)
+  
   
 The following methods may be specified:
 
@@ -1286,6 +1300,17 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname

   
  
+
+ 
+  none
+  
+   
+The server must not prompt the client for an authentication
+exchange. (This does not prohibit client certificate authentication
+   via TLS, nor GSS authentication via its encrypted 
transport.)
+   
+  
+ 
 
   
   
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 59c3575cc3..f789bc7ec3 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -902,9 +902,14 @@ check_expected_areq(AuthRequest areq, PGconn *conn)
{
case AUTH_REQ_OK:
/*
-* Check to make sure we've actually finished 
our exchange.
+* Check to make sure we've actually finished 
our exchange (or
+* else that the user has allowed an 
authentication-less
+* connection).
+*
+* TODO: how should !auth_required interact 
with an incomplete
+* SCRAM exchange?
 */
-   if (conn->client_finished_auth)
+   if (!conn->auth_required || 
conn->client_finished_auth)
break;
 
/*
diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index f2579ff7ee..7d5bf337f6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1265,17 +1265,65 @@ connectOptions2(PGconn *conn)
if (conn->require_auth)
{
char   *s = conn->require_auth;
-   boolmore = true;
+   boolfirst, more;
+   boolnegated = false;
+
+   /*
+* By default, start from an empty set of allowed options and 
add to it.
+*/
+   conn->auth_required = true;
+   conn->allowed_auth_methods = 0;
 
-   while (more)
+   for (first = true, more = true; more; first = false)
{
-   char   

Re: Switching XLog source from archive to streaming when primary available

[Cary Huang]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

Hello

I tested this patch in a setup where the standby is in the middle of 
replicating and REDOing primary's WAL files during a very large data insertion. 
During this time, I keep killing the walreceiver process to cause a stream 
failure and force standby to read from archive. The system will restore from 
archive for "wal_retrieve_retry_interval" seconds before it attempts to steam 
again. Without this patch, once the streaming is interrupted, it keeps reading 
from archive until standby reaches the same consistent state of primary and 
then it will switch back to streaming again. So it seems that the patch does 
the job as described and does bring some benefit during a very large REDO job 
where it will try to re-stream after restoring some WALs from archive to speed 
up this "catch up" process. But if the recovery job is not a large one, PG is 
already switching back to streaming once it hits consistent state.

thank you

Cary Huang
HighGo Software Canada

Re: pg_auth_members.grantor is bunk

[Robert Haas]

On Mon, Jun 6, 2022 at 7:41 PM Stephen Frost  wrote:
> Thankfully, at least from my reading, the spec isn't all that
> complicated on this particular point.  The spec talks about "role
> authorization descriptor"s and those are "created with role name,
> grantee, and grantor" and then further says "redundant duplicate role
> authorization descriptors are destroyed", presumably meaning that the
> entire thing has to be identical.  In other words, yeah, the PK should
> include the grantor.  There's a further comment that the 'set of
> involved grantees' is the union of all the 'grantees', clearly
> indicating that you can have multiple GRANT 'foo' to 'bar's with
> distinct grantees.
>
> In terms of how that's then used, yeah, it's during REVOKE because a
> REVOKE is only able to 'find' role authorization descriptors which match
> the triple of role revoked, grantee, grantor (though there's a caveat in
> that the 'grantor' role could be the current role, or the current user).

What is supposed to happen if someone tries to execute DROP ROLE on a
role that has previously been used as a grantor?

Consider:

create role foo;
create role bar;
create role baz;
grant foo to bar granted by baz;
drop role baz;

Upthread, I proposed that "drop role baz" should fail here, but
there's at least one other option: it could silently remove the grant,
as we would do if either foo or bar were dropped. The situation is not
quite comparable, though: a grant from foo to bar makes no logical
sense if either of those roles cease to exist, but it does make at
least some sense if baz ceases to exist. Therefore I think someone
could argue either for an error or for removing the grant -- or
possibly even for some other behavior, though the other behaviors that
I can think of don't make much sense in a world where the primary key
of pg_auth_members is (roleid, member, grantor).

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

Re: pg_upgrade (12->14) fails on aggregate

[Justin Pryzby]

On Fri, Jun 24, 2022 at 11:43:18PM +0500, Andrey Borodin wrote:
> > On 23 Jun 2022, at 04:58, Justin Pryzby  wrote:
> > 
> > On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
> >> Robert Haas  writes:
> >>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby  
> >>> wrote:
>  To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
> >> 
> >>> Extensions can be installed into pg_catalog, but they can't get
> >>> low-numbered OIDs.
> >> 
> >> Exactly.  (To be clear, I had in mind writing something involving
> >> FirstNormalObjectId, not that you should put literal "16384" in the
> >> code.)
> > 
> > Actually, 16384 is already used in two other places in check.c, so ...
> 
> Yes, but it's a third copy of the comment ("* The query below hardcodes 
> FirstNormalObjectId as 16384 rather than") across the file.
> 
> Also, we can return slightly more information about found objects. For 
> example, operator will look like "operator: ||". At least we can get nspname 
> and oid. And, maybe return type for aggregator and leftarg\rightarg types for 
> operator?

But what I wrote already shows what you want.

In database: postgres
  aggregate: public.array_accum(anyelement)
  operator: public.!@#(anyarray,anyelement)

In my testing, this works great - it shows what you need to put in your DROP
command.  If you try it and still wanted the OID, I'll add it for consistency
with check_for_user_defined_{encoding_conversions,postfix_ops}

> BTW comment /* Before v11, used proisagg=true, and afterwards uses 
> prokind='a' */ seems interesting, but irrelevant. We join with pg_aggregate 
> anyway.

Yes, that's why the query doesn't need to include that.

Something is broken in my old clusters and I can't test all the upgrades right
now, but this is my latest.
>From 87132a8eb7310c4f00d33ea09d97fab481ea1173 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH] WIP: pg_upgrade --check: detect old polymorphics from pre-14

These fail when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check, and not after dumping the cluster and in the middle of
restoring it.

CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
CREATE OPERATOR !@# (PROCEDURE = array_append, LEFTARG=anyarray, rightarg=anyelement);

See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
 src/bin/pg_upgrade/check.c | 134 +
 1 file changed, 134 insertions(+)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..8b8509b6aa5 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
 static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
 static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster);
+static void check_for_old_polymorphics(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
 	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
 		check_for_user_defined_postfix_ops(&old_cluster);
 
+	/*
+	 * PG 14 changed polymorphic functions from anyarray to anycompatiblearray.
+	 */
+	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+		check_for_old_polymorphics(&old_cluster);
+
 	/*
 	 * Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
 	 * supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,133 @@ check_proper_datallowconn(ClusterInfo *cluster)
 }
 
 
+/*
+ *	check_for_old_polymorphics()
+ *
+ *	Make sure nothing is using old polymorphic functions with
+ *	anyarray/anyelement rather than the new anycompatible variants.
+ */
+static void
+check_for_old_polymorphics(ClusterInfo *cluster)
+{
+	PGresult   *res;
+	FILE	   *script = NULL;
+	char		output_path[MAXPGPATH];
+	PQExpBufferData old_polymorphics;
+
+	initPQExpBuffer(&old_polymorphics);
+
+	appendPQExpBufferStr(&old_polymorphics,
+		"'array_append(anyarray,anyelement)', "
+		"'array_cat(anyarray,anyarray)', "
+		"'array_prepend(anyelement,anyarray)', "
+		"'array_remove(anyarray,anyelement)', "
+		"'array_replace(anyarray,anyelement,anyelement)' ");
+
+	if (old_cluster.major_version >= 9500)
+		appendPQExpBufferStr(&old_polymorphics,
+			", "
+			"'array_position(anyarray,anyelement)', "
+			"'array_position(anyarray,anyelement,integer)', "
+			"'array_positions(anyarray,anyelement)', "
+			"'width_bucket(anyelement,anyarray)' ");
+
+	prep_status("Checking for use of old polymorphic functions");
+
+	snprintf(output_path, sizeof(output_path), "%s/%s",
+			 log_opts.basedir,
+			 "databases_with_old_polymorphics.txt");
+
+	for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+	{
+		bo

Re: pg_auth_members.grantor is bunk

[David G. Johnston]

On Fri, Jun 24, 2022 at 1:19 PM Robert Haas  wrote:

> On Mon, Jun 6, 2022 at 7:41 PM Stephen Frost  wrote:
> >
> > In terms of how that's then used, yeah, it's during REVOKE because a
> > REVOKE is only able to 'find' role authorization descriptors which match
> > the triple of role revoked, grantee, grantor (though there's a caveat in
> > that the 'grantor' role could be the current role, or the current user).
>
> What is supposed to happen if someone tries to execute DROP ROLE on a
> role that has previously been used as a grantor?
>
> Upthread, I proposed that "drop role baz" should fail here
>

I concur with this.

I think that the grantor owns the grant, and that REASSIGNED OWNED should
be able to move those grants to someone else.

By extension, DROP OWNED should remove them.

David J.

Re: pg_auth_members.grantor is bunk

[Robert Haas]

On Fri, Jun 24, 2022 at 4:30 PM David G. Johnston
 wrote:
>> Upthread, I proposed that "drop role baz" should fail here
>
> I concur with this.
>
> I think that the grantor owns the grant, and that REASSIGNED OWNED should be 
> able to move those grants to someone else.
>
> By extension, DROP OWNED should remove them.

Interesting. I hadn't thought about changing the behavior of DROP
OWNED BY and REASSIGN OWNED BY. A quick experiment supports your
interpretation:

rhaas=# grant select on table foo to bar;
GRANT
rhaas=# revoke select on table foo from bar;
REVOKE
rhaas=# grant select on table foo to bar with grant option;
GRANT
rhaas=# set role bar;
SET
rhaas=> grant select on table foo to baz;
GRANT
rhaas=> reset role;
RESET
rhaas=# drop role bar;
ERROR:  role "bar" cannot be dropped because some objects depend on it
DETAIL:  privileges for table foo
rhaas=# drop owned by bar;
DROP OWNED
rhaas=# drop role bar;
DROP ROLE

So, privileges on tables (and presumably all other SQL objects)
already work the way that you propose here. If we choose to make role
memberships work in some other way then the two will be inconsistent.
Probably we shouldn't do that. There is still the question of what the
SQL specification says about this, but I would guess that it mandates
the same behavior for all kinds of privileges rather than treating
role memberships and table permissions in different ways. I could be
wrong, though.

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

Re: [PATCH] Optimize json_lex_string by batching character copying

[Zhihong Yu]

Hi,
Looking at the patch,

+   if (copyable_characters_length)
+   {
+   /* flush copyable characters */
+   appendBinaryStringInfo(
+  lex->strval,
+  s - copyable_characters_length,
+  copyable_characters_length);
+
+   }
break;

I wonder why copyable_characters_length is not reset after flushing.

Cheers

Re: Future Postgres 15 and Clang 15

[Thomas Munro]

On Fri, Jun 24, 2022 at 8:35 PM Fabien COELHO  wrote:
> Just a note/reminder that "seawasp" has been unhappy for some days now
> because of yet another change in the unstable API provided by LLVM:

Hi Fabien,

Yeah, I've started on the changes needed for opaque pointers (that's
the change that's been generating warnings since LLVM14, and now
aborts in LLVM15), but I haven't figured out all the puzzles yet.  I
will have another go at this this weekend and then post what I've got,
to show where I'm stuck.

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2022-06-23%2023%3A18%3A17
>
>   llvmjit.c:1115:50: error: use of undeclared identifier 
> 'LLVMJITCSymbolMapPair'
>  LLVMOrcCSymbolMapPairs symbols = 
> palloc0(sizeof(LLVMJITCSymbolMapPair) * LookupSetSize);
>
>   llvmjit.c:1233:81: error: too few arguments to function call, expected 3, 
> have 2
>  ref_gen = 
> LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);

Ah yes, I hadn't seen that one yet.  That function grew a "Dispose"
argument, which we can just pass NULL for, at a guess:

https://github.com/llvm/llvm-project/commit/14b7c108a2bf46541efc3a5c9cbd589b3afc18e6

> The question is: should pg 15 try to be clang 15 ready? I'm afraid yes, as
> LLVM does 2 releases per year, so clang 15 should come out this Fall,
> together with pg 15. Possibly other changes will come before the
> releases:-/

OK let's try to get a patch ready first and then see what we can do.
I'm more worried about code that compiles OK but then crashes or gives
wrong query results (like the one for 9b4e4cfe)  than I am about code
that doesn't compile at all (meaning no one can actually ship it!).  I
think the way our two projects' release cycles work, there will
occasionally be short periods where we can't use their very latest
release, but we can try to avoid that...

Core dump in range_table_mutator()

[Tom Lane]

Commit 64919aaab made pull_up_simple_subquery set rte->subquery = NULL
after doing the deed, so that we don't waste cycles copying a
now-useless subquery tree around.  I discovered today while
working on another patch that if you invoke query_tree_mutator
or range_table_mutator on the whole Query after that point,
range_table_mutator dumps core, because it's expecting subquery
links to never be NULL.  There's apparently noplace in our core
code that does that today, but I'm a bit surprised we've not heard
complaints from anyone else.  I propose to do this to harden it:

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 876f84dd39..8d58265010 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3788,7 +3788,9 @@ range_table_mutator(List *rtable,
 /* we don't bother to copy eref, aliases, etc; OK? */
 break;
 case RTE_SUBQUERY:
-if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+/* In the planner, subquery is null if it's been flattened */
+if (!(flags & QTW_IGNORE_RT_SUBQUERIES) &&
+rte->subquery != NULL)
 {
 CHECKFLATCOPY(newrte->subquery, rte->subquery, Query);
 MUTATE(newrte->subquery, newrte->subquery, Query *);


regards, tom lane

Re: [PATCH] Optimize json_lex_string by batching character copying

[Jelte Fennema]

> +           if (copyable_characters_length)
> +           {
> +               /* flush copyable characters */
> +               appendBinaryStringInfo(
> +                                      lex->strval,
> +                                      s - copyable_characters_length,
> +                                      copyable_characters_length);
> +
> +           }
>            break;
> 
> I wonder why copyable_characters_length is not reset after flushing.

It breaks from the loop right after. So copyable_characters_length isn't used 
again and thus resetting is not necessary. But I agree this could use a comment.

Re: Implement hook for self-join simplification

[Leif Harald Karlsen]

Hi Andrey,


Thank you for the quick answer, and for the pointer to the patch! This looks 
like just the thing I need!


On a more general note: What would, in general, be the best way to implement 
such optimizations? Is there a good way to do this as an extension, or is a 
patch the preferred way?

Kind regards,
Leif Harald Karlsen
Senior Lecturer
Department of Informatics
University of Oslo

From: Andrey Lepikhov 
Sent: 24 June 2022 19:27:50
To: Leif Harald Karlsen; pgsql-hackers@lists.postgresql.org
Subject: Re: Implement hook for self-join simplification

On 24/6/2022 18:58, Leif Harald Karlsen wrote:
> I have a made a small deductive database on top of PostgreSQL for
> educational/research purposes. In this setting, due to certain
> VIEW-constructions, queries often end up being self-joins on primary
> keys, e.g.:
> SELECT t1.id, t2.val
> FROM t AS t1 JOIN t AS t2 USING (id);
>
> where t(id) is a primary key. This query is equivalent to the much more
> efficient:
> SELECT id, val FROM t AS t1;
>
> However, PostgreSQL currently does not seem to implement this
> simplification. Therefore, I have looked into writing an extension that
> performs this, but I am struggling a bit with finding out when this
> simplification should be done, i.e. which hook I should implement.
It is true, but you can use a proposed patch that adds such
functionality [1].

I tried to reproduce your case:
CREATE TABLE t(id int PRIMARY KEY, val text);
explain verbose
SELECT t1.id, t2.val FROM t AS t1 JOIN t AS t2 USING (id);

With this patch you will get a plan:
  Seq Scan on public.t t2
Output: t2.id, t2.val
Filter: (t2.id IS NOT NULL)

The approach, implemented in this patch looks better because removes
self-joins on earlier stage than the path generation stage. Feel free to
use it in your research.

[1]
https://www.postgresql.org/message-id/a1d6290c-44e0-0dfc-3fca-66a68b310...@postgrespro.ru

--
regards,
Andrey Lepikhov
Postgres Professional

Hardening PostgreSQL via (optional) ban on local file system access

[Hannu Krosing]

Hi Pgsql-Hackers

As part of ongoing work on PostgreSQL  security hardening we have
added a capability to disable all file system access (COPY TO/FROM
[PROGRAM] , pg_*file*() functions, lo_*() functions
accessing files, etc) in a way that can not be re-enabled without
already having access to the file system. That is via a flag which can
be set only in postgresql.conf or on the command line.

Currently the file system access is controlled via being a SUPREUSER
or having the pg_read_server_files, pg_write_server_files and
pg_execute_server_program roles. The problem with this approach is
that it will not stop an attacker who has managed to become the
PostgreSQL  SUPERUSER from  breaking out of the server to reading and
writing files and running programs in the surrounding container, VM or
OS.

If we had had this then for example the infamous 2020 PgCrypto worm
[1] would have been much less likely to succeed.

So here are a few questions to get discussion started.

1) would it be enough to just disable WRITING to the filesystem (COPY
... TO ..., COPY TO ... PROGRAM ...) or are some reading functions
also potentially exploitable or at least making attackers life easier
?
2) should configuration be all-or-nothing or more fine-tunable (maybe
a comma-separated list of allowed features) ?
3) should this be back-patched (we can provide batches for all
supported PgSQL versions)
4) We should likely start with this flag off, but any paranoid (read -
good and security conscious)  DBA can turn it on.
5) Which file access functions should be in the unsafe list -
pg_current_logfile is likely safe as is pg_relation_filenode, but
pg_ls_dir likely is not. some subversions might be ok again, like
pg_ls_waldir ?
6) or should we control it via disabling the pg_*_server_* roles for
different take of configuration from 5) ?

As I said, we are happy to provide patches (and docs, etc) for all the
PostgreSQL versions we decide to support here.

Best Regards
Hannu


-
[1] 
https://www.securityweek.com/pgminer-crypto-mining-botnet-abuses-postgresql-distribution

Re: SLRUs in the main buffer pool - Page Header definitions

[Shawn Debnath]

On Thu, Jun 23, 2022 at 06:06:48PM -0700, Andres Freund wrote:

> > You are correct that we wouldn’t need to rely on the pd_flag bit to
> > determine page type for any access to a page where we come top down
> > following the hierarchy. However, for the purpose of debugging “from the
> > bottom up” it would be critical to know what type of page is being read in a
> > system with multiple page header types.
> 
> That doesn't seem to justify using a bit on the page. Wouldn't it suffice to
> add such information to the BufferDesc?

The goal for the bit in pd_flags is to help identify what page header 
should be present if one were to be looking at the physical page outside 
of the database. One example that comes to mind is pg_upgrade.  There 
are other use cases where physical consistency checks could be applied, 
again outside of a running database.

On Thu, Jun 23, 2022 at 04:27:33PM -0400, Robert Haas wrote:

> I think that it's not worth introducing a new page header format to
> save 10 bytes per page. Keeping things on the same format is likely to
> save more than the minor waste of space costs.

Yeah, I think we are open to both approaches, though we believe it would 
be cleaner to get started with a targeted page header for the new code.  
But do understand having to understand/translate/deal with two page 
header types might not be worth the savings in space.

If we stick with the current page header, of course, changes to pd_flag 
won't be necessary anymore.

Stepping back, it seems like folks are okay with introducing a page 
header to current SLRU components and that we are leaning towards 
sticking with the default one for now. We can proceed with this 
approach, and if needed, change it later if more folks chime in.

Cheers.

-- 
Shawn Debnath
Amazon Web Services (AWS)

Re: SLRUs in the main buffer pool - Page Header definitions

[Andres Freund]

Hi,

On 2022-06-24 22:19:33 +, Shawn Debnath wrote:
> On Thu, Jun 23, 2022 at 06:06:48PM -0700, Andres Freund wrote:
>
> > > You are correct that we wouldn’t need to rely on the pd_flag bit to
> > > determine page type for any access to a page where we come top down
> > > following the hierarchy. However, for the purpose of debugging “from the
> > > bottom up” it would be critical to know what type of page is being read 
> > > in a
> > > system with multiple page header types.
> >
> > That doesn't seem to justify using a bit on the page. Wouldn't it suffice to
> > add such information to the BufferDesc?
>
> The goal for the bit in pd_flags is to help identify what page header
> should be present if one were to be looking at the physical page outside
> of the database. One example that comes to mind is pg_upgrade.  There
> are other use cases where physical consistency checks could be applied,
> again outside of a running database.

Outside the database you'll know the path to the file, which will tell you
it's not another kind of relation.

This really makes no sense to me. We don't have page flags indicating whether
a page is a heap, btree, visibility, fms whatever kind of page either. On a
green field, it'd make sense to have such information in a metapage at the
start of each relation - but not on each page.


> On Thu, Jun 23, 2022 at 04:27:33PM -0400, Robert Haas wrote:
>
> > I think that it's not worth introducing a new page header format to
> > save 10 bytes per page. Keeping things on the same format is likely to
> > save more than the minor waste of space costs.
>
> Yeah, I think we are open to both approaches, though we believe it would
> be cleaner to get started with a targeted page header for the new code.
> But do understand having to understand/translate/deal with two page
> header types might not be worth the savings in space.

Not sure if that changes anything, but it's maybe worth noting that we already
have some types of pages that don't use the full page header (at least
freespace.c, c.f. buffer_std argument to MarkBufferDirtyHint()). I can see an
argument for shrinking the "uniform" part of the page header, and pushing more
things down into AMs. But I think that'd need to change the existing code, not
just introduce something new for new code.


> Stepping back, it seems like folks are okay with introducing a page
> header to current SLRU components and that we are leaning towards
> sticking with the default one for now. We can proceed with this
> approach, and if needed, change it later if more folks chime in.

I think we're clearly going to have to do that at some point not too far
away. There's just too many capabilities that are made harder by not having
that information for SLRU pages. That's not to say that it's a prerequisite to
moving SLRUs into the buffer pool (using a hack like Thomas did until the page
header is introduced). Both are complicated enough projects on their own. I
also could see adding the page header before moving SLRUs in the buffer pool,
there isn't really a hard dependency.

Greetings,

Andres Freund

Re: Hardening PostgreSQL via (optional) ban on local file system access

[David G. Johnston]

On Fri, Jun 24, 2022 at 3:08 PM Hannu Krosing  wrote:

>
> 1) would it be enough to just disable WRITING to the filesystem (COPY
> ... TO ..., COPY TO ... PROGRAM ...) or are some reading functions
> also potentially exploitable or at least making attackers life easier
> ?
>

I would protect read paths as well as write ones.

Though ISTM reading would need to be more fine-grained - raw filesystem
reads and system reads (i.e., something like get_raw_page(...))

2) should configuration be all-or-nothing or more fine-tunable (maybe
> a comma-separated list of allowed features) ?
>

First pass, all-or-nothing, focus on architecture and identification.
Ideally we can then easily go in and figure out specific capabilities that
need to be enumerated should we desire.  Or, as noted below, figure out how
to do a DBA administered whitelist.

3) should this be back-patched (we can provide batches for all
> supported PgSQL versions)
>

I would love to in theory, but to do this right I suspect that the amount
of desirable refactoring would make doing so prohibitive.


> 4) We should likely start with this flag off, but any paranoid (read -
> good and security conscious)  DBA can turn it on.
>

In the end the vast majority of our users will have the decision as to the
default state of this decided for them by their distribution or service
provider.  I'm fine with having build-from-source users get the more
permissive default.

5) Which file access functions should be in the unsafe list -
> pg_current_logfile is likely safe as is pg_relation_filenode, but
> pg_ls_dir likely is not. some subversions might be ok again, like
> pg_ls_waldir ?
> 6) or should we control it via disabling the pg_*_server_* roles for
> different take of configuration from 5) ?
>

I would suggest neither: we should funnel user-initiated access to the
filesystem, for read and write, through its own API that will simply
prevent all writes (and maybe reads) based upon this flag.  If we can
somehow enforce that a C coded extension also use this API we should do so,
but IIUC that is not possible.
This basically puts things in a "default deny" mode.  I do think that we
need something that permits a filesystem user to then say "except these"
(i.e., a whitelist).  The thing added to the whitelist should be available
in the PostgreSQL log file when the API rejects the attempt to access the
filesystem.  Unfortunately, at the moment I hit a brick wall when thinking
exactly how that could be accomplished.  At least, in a minimal/zero trust
type of setup.  Having the API access include the module, function, and
version making the request and having a semantic versioning based whitelist
(like, e.g., npm) would work sufficiently well in a "the only ones that get
installed on the server are trusted to play by the rules" setup.

Probably over-engineering it like I have a tendency to do, but some food
for thought nonetheless.

David J.

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Nathan Bossart]

On Sat, Jun 25, 2022 at 12:08:13AM +0200, Hannu Krosing wrote:
> As part of ongoing work on PostgreSQL  security hardening we have
> added a capability to disable all file system access (COPY TO/FROM
> [PROGRAM] , pg_*file*() functions, lo_*() functions
> accessing files, etc) in a way that can not be re-enabled without
> already having access to the file system. That is via a flag which can
> be set only in postgresql.conf or on the command line.
> 
> Currently the file system access is controlled via being a SUPREUSER
> or having the pg_read_server_files, pg_write_server_files and
> pg_execute_server_program roles. The problem with this approach is
> that it will not stop an attacker who has managed to become the
> PostgreSQL  SUPERUSER from  breaking out of the server to reading and
> writing files and running programs in the surrounding container, VM or
> OS.

There was some recent discussion in this area you might be interested in
[0].

[0] https://postgr.es/m/20220520225619.GA876272%40nathanxps13

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

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Andres Freund]

Hi,

On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
> Currently the file system access is controlled via being a SUPREUSER
> or having the pg_read_server_files, pg_write_server_files and
> pg_execute_server_program roles. The problem with this approach is
> that it will not stop an attacker who has managed to become the
> PostgreSQL  SUPERUSER from  breaking out of the server to reading and
> writing files and running programs in the surrounding container, VM or
> OS.

If a user has superuser rights, they automatically can execute arbitrary
code. It's that simple. Removing roles isn't going to change that. Our code
doesn't protect against C functions mismatching their SQL level
definitions. With that you can do a lot of things.


> If we had had this then for example the infamous 2020 PgCrypto worm
> [1] would have been much less likely to succeed.

Meh.


> So here are a few questions to get discussion started.
> 
> 1) would it be enough to just disable WRITING to the filesystem (COPY
> ... TO ..., COPY TO ... PROGRAM ...) or are some reading functions
> also potentially exploitable or at least making attackers life easier
> ?
> 2) should configuration be all-or-nothing or more fine-tunable (maybe
> a comma-separated list of allowed features) ?
> 3) should this be back-patched (we can provide batches for all
> supported PgSQL versions)

Err, what?

> 4) We should likely start with this flag off, but any paranoid (read -
> good and security conscious)  DBA can turn it on.
> 5) Which file access functions should be in the unsafe list -
> pg_current_logfile is likely safe as is pg_relation_filenode, but
> pg_ls_dir likely is not. some subversions might be ok again, like
> pg_ls_waldir ?
> 6) or should we control it via disabling the pg_*_server_* roles for
> different take of configuration from 5) ?
> 
> As I said, we are happy to provide patches (and docs, etc) for all the
> PostgreSQL versions we decide to support here.

I don't see anything here that provides a meaningful increase in security.

Greetings,

Andres Freund

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Hannu Krosing]

On Sat, Jun 25, 2022 at 1:13 AM Andres Freund  wrote:
>
> Hi,
>
> On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
> > Currently the file system access is controlled via being a SUPREUSER
> > or having the pg_read_server_files, pg_write_server_files and
> > pg_execute_server_program roles. The problem with this approach is
> > that it will not stop an attacker who has managed to become the
> > PostgreSQL  SUPERUSER from  breaking out of the server to reading and
> > writing files and running programs in the surrounding container, VM or
> > OS.
>
> If a user has superuser rights, they automatically can execute arbitrary
> code. It's that simple. Removing roles isn't going to change that. Our code
> doesn't protect against C functions mismatching their SQL level
> definitions. With that you can do a lot of things.

Are you claiming that one can manipulate PostgreSQL to do any file
writes directly by manipulating pg_proc to call the functions "in  a
wrong way" ?

My impression was that this was largely fixed via disabling the old
direct file calling convention, but then again I did not pay much
attention at that time :)

So your suggestion would be to also include disabling access to at
least pg_proc for creating C and internal functions and possibly some
other system tables to remove this threat ?

Cheers
Hannu

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Hannu Krosing]

On Sat, Jun 25, 2022 at 1:23 AM Hannu Krosing  wrote:

> My impression was that this was largely fixed via disabling the old
> direct file calling convention, but then again I did not pay much
> attention at that time :)

I meant of course direct FUNCTION calling convention (Version 0
Calling Conventions)

-- Hannu

Re: Hardening PostgreSQL via (optional) ban on local file system access

[David G. Johnston]

On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:

> Hi,
>
> On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
> > Currently the file system access is controlled via being a SUPREUSER
> > or having the pg_read_server_files, pg_write_server_files and
> > pg_execute_server_program roles. The problem with this approach is
> > that it will not stop an attacker who has managed to become the
> > PostgreSQL  SUPERUSER from  breaking out of the server to reading and
> > writing files and running programs in the surrounding container, VM or
> > OS.
>
> If a user has superuser rights, they automatically can execute arbitrary
> code. It's that simple. Removing roles isn't going to change that. Our code
> doesn't protect against C functions mismatching their SQL level
> definitions. With that you can do a lot of things.
>
>
Using only psql connected by the postgres role, without touching the
filesystem to bootstrap your attack, how would this be done?  If you
specify CREATE FUNCTION ... LANGUAGE c you have to supply filename
references, not a code body and you won't have been able to put that code
on the server.

We should be capable of having the core server be inescapable to the
filesystem for a superuser logged in remotely.  All such access they can do
with the filesystem would be mediated by controlled code/APIs.

C-based extensions would be an issue without a solution that does provide
an inescapable sandbox aside from going through our API.  Which I suspect
is basically impossible given our forked process driven execution model.

David J.


David J.

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Gurjeet Singh]

On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:

> > 3) should this be back-patched (we can provide batches for all
> > supported PgSQL versions)
>
> Err, what?

Translation: Backpatching these changes to any stable versions will
not be acceptable (per the project versioning policy [1]), since these
changes would be considered new feature. These changes can break
installations, if released in a minor version.

[1]: https://www.postgresql.org/support/versioning/

Best regards,
Gurjeet
http://Gurje.et

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Hannu Krosing]

My understanding was that unless activated by admin these changes
would change nothing.

And they would be (borderline :) ) security fixes

And the versioning policy link actually does not say anything about
not adding features to older versions (I know this is the policy, just
pointing out the info in not on that page).

On Sat, Jun 25, 2022 at 1:46 AM Gurjeet Singh  wrote:
>
> On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> > On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
>
> > > 3) should this be back-patched (we can provide batches for all
> > > supported PgSQL versions)
> >
> > Err, what?
>
> Translation: Backpatching these changes to any stable versions will
> not be acceptable (per the project versioning policy [1]), since these
> changes would be considered new feature. These changes can break
> installations, if released in a minor version.
>
> [1]: https://www.postgresql.org/support/versioning/
>
> Best regards,
> Gurjeet
> http://Gurje.et

Re: Hardening PostgreSQL via (optional) ban on local file system access

[David G. Johnston]

On Friday, June 24, 2022, Gurjeet Singh  wrote:

> On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> > On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
>
> > > 3) should this be back-patched (we can provide batches for all
> > > supported PgSQL versions)
> >
> > Err, what?
>
> Translation: Backpatching these changes to any stable versions will
> not be acceptable (per the project versioning policy [1]), since these
> changes would be considered new feature. These changes can break
> installations, if released in a minor version.
>
>
No longer having the public schema in the search_path was a feature that
got back-patched, with known bad consequences, without any way for the DBA
to voice their opinion on the matter.  This proposal seems similar enough
to at least ask the question, with full DBA control and no known bad
consequences.

David J.

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Hannu Krosing]

The old versions should definitely not have it turned on by default. I
probably was not as clear as I thought in bringing out that point..

For upcoming next ones the distributors may want to turn it on for
some more security-conscious ("enterprize") distributions.

-- Hannu

On Sat, Jun 25, 2022 at 2:08 AM David G. Johnston
 wrote:
>
>
>
> On Friday, June 24, 2022, Gurjeet Singh  wrote:
>>
>> On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
>> > On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
>>
>> > > 3) should this be back-patched (we can provide batches for all
>> > > supported PgSQL versions)
>> >
>> > Err, what?
>>
>> Translation: Backpatching these changes to any stable versions will
>> not be acceptable (per the project versioning policy [1]), since these
>> changes would be considered new feature. These changes can break
>> installations, if released in a minor version.
>>
>
> No longer having the public schema in the search_path was a feature that got 
> back-patched, with known bad consequences, without any way for the DBA to 
> voice their opinion on the matter.  This proposal seems similar enough to at 
> least ask the question, with full DBA control and no known bad consequences.
>
> David J.
>

Re: [PATCH] Optimize json_lex_string by batching character copying

[Andres Freund]

Hi,

On 2022-06-24 08:47:09 +, Jelte Fennema wrote:
> To test performance of this change I used COPY BINARY from a JSONB table
> into another, containing fairly JSONB values of ~15kB.

This will have a lot of other costs included (DML is expensive). I'd suggest
storing the json in a text column and casting it to json[b], with a filter
ontop of the json[b] result that cheaply filters it away. That should end up
spending nearly all the time somewhere around json parsing.

It's useful for things like this to include a way for others to use the same
benchmark...

I tried your patch with:

DROP TABLE IF EXISTS json_as_text;
CREATE TABLE json_as_text AS SELECT (SELECT json_agg(row_to_json(pd)) as t FROM 
pg_description pd) FROM generate_series(1, 100);
VACUUM FREEZE json_as_text;

SELECT 1 FROM json_as_text WHERE jsonb_typeof(t::jsonb) = 'not me';

Which the patch improves from 846ms to 754ms (best of three). A bit smaller
than your improvement, but still nice.


I think your patch doesn't quite go far enough - we still end up looping for
each character, have the added complication of needing to flush the
"buffer". I'd be surprised if a "dedicated" loop to see until where the string
last isn't faster.  That then obviously could be SIMDified.


Separately, it seems pretty awful efficiency / code density wise to have the
NULL checks for ->strval all over. Might be worth forcing json_lex() and
json_lex_string() to be inlined, with a constant parameter deciding whether
->strval is expected. That'd likely be enough to get the compiler specialize
the code for us.


Might also be worth to maintain ->strval using appendBinaryStringInfoNT().

Greetings,

Andres Freund

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Gurjeet Singh]

(fixed your top-posting)

On Fri, Jun 24, 2022 at 4:59 PM Hannu Krosing  wrote:
> On Sat, Jun 25, 2022 at 1:46 AM Gurjeet Singh  wrote:
> >
> > On Fri, Jun 24, 2022 at 4:13 PM Andres Freund  wrote:
> > > On 2022-06-25 00:08:13 +0200, Hannu Krosing wrote:
> >
> > > > 3) should this be back-patched (we can provide batches for all
> > > > supported PgSQL versions)
> > >
> > > Err, what?
> >
> > Translation: Backpatching these changes to any stable versions will
> > not be acceptable (per the project versioning policy [1]), since these
> > changes would be considered new feature. These changes can break
> > installations, if released in a minor version.
> >
> > [1]: https://www.postgresql.org/support/versioning/
>
> My understanding was that unless activated by admin these changes
> would change nothing.
>
> And they would be (borderline :) ) security fixes
>
> And the versioning policy link actually does not say anything about
> not adding features to older versions (I know this is the policy, just
> pointing out the info in not on that page).

I wanted to be sure before I mentioned it, and also because I've been
away from the community for a few years [1], so I too searched the
page for any relevant mentions of the word "feature" on that page.
While you're correct that the policy does not address/prohibit
addition of new features in minor releases, but the following line
from the policy comes very close to saying it, without actually saying
it.

> ... PostgreSQL minor releases fix only frequently-encountered bugs, security 
> issues, and data corruption problems to reduce the risk associated with 
> upgrading ...

Like I recently heard a "wise one" recently say: "oh those [Postgres]
docs are totally unclear[,] but they're technically correct".

BTW, the "Translation" bit was for folks new to, or not familiar with,
community and its lingo; I'm sure you already knew what Andres meant
:-)

[1]: I'll milk the "I've been away from the community for a few years"
excuse for as long as possible ;-)

Best regards,
Gurjeet
http://Gurje.et

Re: Add non-blocking version of PQcancel

[Justin Pryzby]

Resending with a problematic email removed from CC...

On Mon, Apr 04, 2022 at 03:21:54PM +, Jelte Fennema wrote:
> 2. Added some extra sleeps to the cancellation test, to remove random 
> failures on FreeBSD.

Apparently there's still an occasional issue.
https://cirrus-ci.com/task/6613309985128448

result 232/352 (error): ERROR:  duplicate key value violates unique constraint 
"ppln_uniqviol_pkey"
DETAIL:  Key (id)=(116) already exists.

This shows that the issue is pretty rare:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/38/3511

-- 
Justin

Re: [PATCH] Optimize json_lex_string by batching character copying

[Andres Freund]

Hi,

On 2022-06-24 17:18:10 -0700, Andres Freund wrote:
> On 2022-06-24 08:47:09 +, Jelte Fennema wrote:
> > To test performance of this change I used COPY BINARY from a JSONB table
> > into another, containing fairly JSONB values of ~15kB.
> 
> This will have a lot of other costs included (DML is expensive). I'd suggest
> storing the json in a text column and casting it to json[b], with a filter
> ontop of the json[b] result that cheaply filters it away. That should end up
> spending nearly all the time somewhere around json parsing.
> 
> It's useful for things like this to include a way for others to use the same
> benchmark...
> 
> I tried your patch with:
> 
> DROP TABLE IF EXISTS json_as_text;
> CREATE TABLE json_as_text AS SELECT (SELECT json_agg(row_to_json(pd)) as t 
> FROM pg_description pd) FROM generate_series(1, 100);
> VACUUM FREEZE json_as_text;
> 
> SELECT 1 FROM json_as_text WHERE jsonb_typeof(t::jsonb) = 'not me';
> 
> Which the patch improves from 846ms to 754ms (best of three). A bit smaller
> than your improvement, but still nice.
> 
> 
> I think your patch doesn't quite go far enough - we still end up looping for
> each character, have the added complication of needing to flush the
> "buffer". I'd be surprised if a "dedicated" loop to see until where the string
> last isn't faster.  That then obviously could be SIMDified.

A naive implementation (attached) of that gets me down to 706ms.

Greetings,

Andres Freund
diff --git i/src/common/jsonapi.c w/src/common/jsonapi.c
index 98e4ef09426..63d92c66aec 100644
--- i/src/common/jsonapi.c
+++ w/src/common/jsonapi.c
@@ -858,10 +858,25 @@ json_lex_string(JsonLexContext *lex)
 		}
 		else if (lex->strval != NULL)
 		{
+			size_t chunklen = 1;
+
 			if (hi_surrogate != -1)
 return JSON_UNICODE_LOW_SURROGATE;
 
-			appendStringInfoChar(lex->strval, *s);
+			while (len + chunklen < lex->input_length)
+			{
+char next = *(s + chunklen);
+
+if (next == '\\' || next == '"' || (unsigned char) next < 32)
+	break;
+
+chunklen++;
+			}
+
+			appendBinaryStringInfo(lex->strval, s, chunklen);
+
+			s += (chunklen - 1);
+			len += (chunklen - 1);
 		}
 	}
 

Re: Hardening PostgreSQL via (optional) ban on local file system access

[Andres Freund]

Hi,

On 2022-06-25 01:23:36 +0200, Hannu Krosing wrote:
> Are you claiming that one can manipulate PostgreSQL to do any file
> writes directly by manipulating pg_proc to call the functions "in  a
> wrong way" ?

Yes.


> My impression was that this was largely fixed via disabling the old
> direct file calling convention, but then again I did not pay much
> attention at that time :)

It got a tad harder, that's all.


> So your suggestion would be to also include disabling access to at
> least pg_proc for creating C and internal functions and possibly some
> other system tables to remove this threat ?

No. I seriously doubt that pursuing this makes sense. Fundamentally, if you
found a way to escalate to superuser, you're superuser. Superuser can create
extensions etc. That's game over. Done.

You can of course make postgres drop a few privileges, to make it harder to
turn escalation-to-superuser into wider access to the whole system. That could
very well make sense - but of course there's quite a few things that postgres
needs to do to work, so there's significant limits to what you can do.

Greetings,

Andres Freund

Re: making relfilenodes 56 bits

[Andres Freund]

Hi,

On 2022-06-24 10:59:25 -0400, Robert Haas wrote:
> A preliminary refactoring that was discussed in the past and was
> originally in 0001 was to move the fields included in BufferTag via
> RelFileNode/Locator directly into the struct. I think maybe it doesn't
> make sense to include that in 0001 as you have it here, but maybe that
> could be 0002 with the main patch to follow as 0003, or something like
> that. I wonder if we can get by with redefining RelFileNode like this
> in 0002:
> 
> typedef struct buftag
> {
> Oid spcOid;
> Oid dbOid;
> RelFileNumber   fileNumber;
> ForkNumber  forkNum;
> } BufferTag;

If we "inline" RelFileNumber, it's probably worth reorder the members so that
the most distinguishing elements come first, to make it quicker to detect hash
collisions. It shows up in profiles today...

I guess it should be blockNum, fileNumber, forkNumber, dbOid, spcOid? I think
as long as blockNum, fileNumber are first, the rest doesn't matter much.


> And then like this in 0003:
> 
> typedef struct buftag
> {
> Oid spcOid;
> Oid dbOid;
> RelFileNumber   fileNumber:56;
> ForkNumber  forkNum:8;
> } BufferTag;

Probably worth checking the generated code / the performance effects of using
bitfields (vs manual maskery). I've seen some awful cases, but here it's at a
byte boundary, so it might be ok.

Greetings,

Andres Freund

Re: Core dump in range_table_mutator()

[Dean Rasheed]

On Fri, 24 Jun 2022 at 22:44, Tom Lane  wrote:
>
> Commit 64919aaab made pull_up_simple_subquery set rte->subquery = NULL
> after doing the deed, so that we don't waste cycles copying a
> now-useless subquery tree around.  I discovered today while
> working on another patch that if you invoke query_tree_mutator
> or range_table_mutator on the whole Query after that point,
> range_table_mutator dumps core, because it's expecting subquery
> links to never be NULL.  There's apparently noplace in our core
> code that does that today, but I'm a bit surprised we've not heard
> complaints from anyone else.  I propose to do this to harden it:
>

Makes sense.

Not directly related to that change ... I think it would be easier to
follow if the CHECKFLATCOPY() was replaced with a separate Assert()
and FLATCOPY() (I had to go and remind myself what CHECKFLATCOPY()
did).

Doing that would allow CHECKFLATCOPY() to be deleted, since this is
the only place that uses it -- every other case knows the node type is
correct before doing a FLATCOPY().

Well almost. The preceding FLATCOPY() of the containing RangeTblEntry
doesn't check the node type, but that could be fixed by using
lfirst_node() instead of lfirst() at the start of the loop, which
would be neater.

Regards,
Dean

Re: Core dump in range_table_mutator()

[Tom Lane]

Dean Rasheed  writes:
> Not directly related to that change ... I think it would be easier to
> follow if the CHECKFLATCOPY() was replaced with a separate Assert()
> and FLATCOPY() (I had to go and remind myself what CHECKFLATCOPY()
> did).
> Doing that would allow CHECKFLATCOPY() to be deleted, since this is
> the only place that uses it -- every other case knows the node type is
> correct before doing a FLATCOPY().

Well, if we want to clean this up a bit rather than just doing the
minimum safe fix ... I spent some time why we were bothering with the
FLATCOPY step at all, rather than just mutating the Query* pointer.
I think the reason is to not fail if the QTW_DONT_COPY_QUERY flag is
set, but maybe we should clear that flag when recursing?

regards, tom lane

Re: Implement hook for self-join simplification

[Andrey Lepikhov]

On 24/6/2022 23:43, Leif Harald Karlsen wrote:
Thank you for the quick answer, and for the pointer to the patch! This 
looks like just the thing I need! 
On a more general note: What would, in general, be the best way to 
implement such optimizations? Is there a good way to do this as an 
extension, or is a patch the preferred way?

According to my experience, it depends on your needings.
For example, self-join-removal feature, or my current project - 
flattening of nested subqueries - is much more optimal to implement as a 
patch, because you can do it so early as possible and can generalize 
parts of the core code and thus, reduce size of your code a lot.
But if you want to use your code with many PG versions, even already 
working in production or you make just a research, without immediate 
practical result - your choice is an extension.


--
regards,
Andrey Lepikhov
Postgres Professional

Re: Future Postgres 15 and Clang 15

[Fabien COELHO]

Hello Thomas,


  llvmjit.c:1233:81: error: too few arguments to function call, expected 3, 
have 2
 ref_gen = 
LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);


Ah yes, I hadn't seen that one yet.  That function grew a "Dispose"
argument, which we can just pass NULL for, at a guess:

https://github.com/llvm/llvm-project/commit/14b7c108a2bf46541efc3a5c9cbd589b3afc18e6


I agree with the guess. Whether the NULL induced semantics is the right 
one is much less obvious…



The question is: should pg 15 try to be clang 15 ready? I'm afraid yes, as
LLVM does 2 releases per year, so clang 15 should come out this Fall,
together with pg 15. Possibly other changes will come before the
releases:-/


OK let's try to get a patch ready first and then see what we can do.
I'm more worried about code that compiles OK but then crashes or gives
wrong query results (like the one for 9b4e4cfe)  than I am about code
that doesn't compile at all (meaning no one can actually ship it!).  I
think the way our two projects' release cycles work, there will
occasionally be short periods where we can't use their very latest
release, but we can try to avoid that...


Yep. The part which would worry me is the code complexity and kludges 
induced by trying to support a moving API. Maybe careful header-handled 
macros can do the trick (eg for an added parameter as above), but I'm 
afraid it cannot always be that simple.


--
Fabien.