Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

so 25. 5. 2024 v 3:29 odesílatel Tom Lane  napsal:

> Pavel Stehule  writes:
> > we can introduce special safe mode started by
> > set enable_direct_variable_read to off;
> > and allowing access to variables only by usage dedicated function
> > (supported by parser) named like variable or pg_variable
>
> Didn't we learn twenty years ago that GUCs that change query
> semantics are an awful idea?  Pick a single access method
> for these things and stick to it.
>

I don't think the proposed GUC exactly changes query semantics - it is
equivalent of plpgsql options: plpgsql_extra_ or #variable_conflict. It
allows us to identify broken queries. And for tools that generates queries
is not problem to wrap reading variable by special pseudo function. The
code where pseudo function will be used should to work with active or
inactive strict mode (related to possibility to use variables).

Sure there is more possibilities, but I don't want to lost the possibility
to write code like

CREATE TEMP VARIABLE _x;

LET _x = 'hello';

DO $$
BEGIN
  RAISE NOTICE '%', _x;
END;
$$;

So I am searching for a way to do it safely, but still intuitive and user
friendly.

Regards

Pavel



>
> regards, tom lane
>

Re: Volatile write caches on macOS and Windows, redux

[Nathan Bossart]

On Thu, Mar 14, 2024 at 01:12:05PM +1300, Thomas Munro wrote:
> Short sales pitch for these patches:
> 
> * the default settings eat data on Macs and Windows
> * nobody understands what wal_sync_method=fsync_writethrough means anyway
> * it's a weird kludge that it affects not only WAL, let's clean that up

I recently started using macOS for hacking on Postgres and noticed this
problem, so I was delighted to find this thread.  I intend to review
further soon, but +1 for improving the default settings.  I think we might
also need some additional fcntl(F_FULLFSYNC) calls in sync_pgdata(),
sync_dir_recurse(), etc., which are used by initdb, pg_basebackup, and
more.

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

Re: Schema variables - new implementation for Postgres 15

[Tom Lane]

Pavel Stehule  writes:
> we can introduce special safe mode started by
> set enable_direct_variable_read to off;
> and allowing access to variables only by usage dedicated function
> (supported by parser) named like variable or pg_variable

Didn't we learn twenty years ago that GUCs that change query
semantics are an awful idea?  Pick a single access method
for these things and stick to it.

regards, tom lane

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

Hi

st 22. 5. 2024 v 19:25 odesílatel Tom Lane  napsal:

> Peter Eisentraut  writes:
> > On 18.05.24 13:29, Alvaro Herrera wrote:
> >> I want to note that when we discussed this patch series at the dev
> >> meeting in FOSDEM, a sort-of conclusion was reached that we didn't want
> >> schema variables at all because of the fact that creating a variable
> >> would potentially change the meaning of queries by shadowing table
> >> columns.  But this turns out to be incorrect: it's_variables_  that are
> >> shadowed by table columns, not the other way around.
>
> > But that's still bad, because seemingly unrelated schema changes can
> > make variables appear and disappear.  For example, if you have
> >   SELECT a, b FROM table1
> > and then you drop column b, maybe the above query continues to work
> > because there is also a variable b.
>
> Yeah, that seems pretty dangerous.  Could we make it safe enough
> by requiring some qualification on variable names?  That is, if
> you mean b to be a variable, then you must write something like
>
> SELECT a, pg_variables.b FROM table1
>
> This is still ambiguous if you use "pg_variables" as a table alias in
> the query, but the alias would win so the query still means what it
> meant before.  Also, table aliases (as opposed to actual table names)
> don't change readily, so I don't think there's much risk of the query
> suddenly meaning something different than it did yesterday.
>

we can introduce special safe mode started by

set enable_direct_variable_read to off;

and allowing access to variables only by usage dedicated function
(supported by parser) named like variable or pg_variable

so it can looks like

select a, pg_variable(myschema.myvar) from table

In this mode, the variables never are readable directly, so there is no
risk of collision and issue mentioned by Peter. And the argument of the
pg_variable pseudo function can be only variable, so risk of possible
collision can be reduced too. The pseudo function pg_variable can be used
in less restrictive mode too, when the user can explicitly show usage of
the variable.

Tom's proposal is already almost supported now. The user can use a
dedicated schema without assigning this schema to search_path. Then a
qualified name should be required.

Can this design be the correct answer for mentioned objections?

 Regards

Pavel



> regards, tom lane
>

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

[Jelte Fennema-Nio]

On Fri, 24 May 2024 at 15:28, Robert Haas  wrote:
>
> On Thu, May 23, 2024 at 4:00 PM Tom Lane  wrote:
> > I don't recall exactly what I thought earlier, but now I think we'd
> > be better off with separate infrastructure.  guc.c is unduly complex
> > already.  Perhaps there are bits of it that could be factored out
> > and shared, but I bet not a lot.
>
> OK. That seems fine to me, but I bet Jelte is going to disagree.

I indeed disagree. I think the effort needed to make guc.c handle
protocol parameters is extremely little. The 0011 patch are all the
changes that are needed to achieve that, and that patch only adds 65
additional lines. And only 15 of those 65 lines actually have to do
anything somewhat weird, to be able to handle the transactionality
discrepancy between protocol parameters and other GUCs. The other 50
lines are (imho) really clean and fit perfectly with the way guc.c is
currently structured (i.e. they add PGC_PROTOCOL and PGC_SU_PROTOCOL
in a really obvious way)

Separating it from the GUC infrastructure will mean we need to
duplicate a lot of the infrastructure. Assuming we don't care about
SHOW or pg_settings (which I agree are not super important), the
things that we would want for protocol parameters to have that guc.c
gives us for free are:
1. Reporting the value of the parameter to the client (done using
ParameterStatus)
2. Parsing and validating of the input, bool, int, enum, etc, but also
check_hook and assign_hook.
3. Logic in all connection poolers to change GUC values to the
client's expected values whenever a server connection is handed off to
a client
4. Permission checking, if we want some protocol extensions to only be
configurable by a highly privileged user

All of those things would have to be duplicated/re-implemented if we
make protocol parameters their own dedicated thing. Doing that work
seems like a waste of time to me, and would imho add much more
complexity than the proposed 65 lines of code in 0011.

Re: Streaming I/O, vectored I/O (WIP)

[Thomas Munro]

On Wed, Nov 29, 2023 at 1:17 AM Thomas Munro  wrote:
> Done.  I like it, I just feel a bit bad about moving the p*v()
> replacement functions around a couple of times already!  I figured it
> might as well be static inline even if we use the fallback (= Solaris
> and Windows).

Just for the record, since I'd said things like the above a few times
while writing about this stuff: Solaris 11.4.69 has gained preadv()
and pwritev().  That's interesting because it means that there will
soon be no liive Unixoid operating systems left without them, and the
fallback code in src/include/port/pg_iovec.h will, in practice, be
only for Windows.  I wondered if that might have implications for how
we code or comment stuff like that, but it still seems to make sense
as we have it.

(I don't think Windows can have a real synchronous implementation; the
kernel knows how to do scatter/gather, a feature implemented
specifically for databases, but only in asynchronous ("overlapped") +
direct I/O mode, a difference I don't know how to hide at this level.
In later AIO work we should be able to use it as intended, but not by
pretending to be Unix like this.)

Re: commitfest.postgresql.org is no longer fit for purpose

[Tom Lane]

Tomas Vondra  writes:
> I personally don't think the FOSDEM triage is a very productive use of
> our time - we go through patches top to bottom, often with little idea
> what the current state of the patch is. We always ran out of time after
> looking at maybe 1/10 of the list.

> Having an in-person discussion about patches would be good, but I think
> we should split the meeting into much smaller groups for that, each
> looking at a different subset. And maybe it should be determined in
> advance, so that people can look at those patches in advance ...

Yeah, subgroups of 3 or 4 people sounds about right.  And definitely
some advance looking to see which patches need discussion.

regards, tom lane

Re: commitfest.postgresql.org is no longer fit for purpose

[Tomas Vondra]

On 5/24/24 22:44, Tom Lane wrote:
> Joe Conway  writes:
>> On 5/24/24 15:45, Tom Lane wrote:
>>> I was *not* proposing doing a regular review, unless of course
>>> somebody really wants to do that.  What I am thinking about is
>>> suggesting how to make progress on patches that are stuck, or in some
>>> cases delivering the bad news that this patch seems unlikely to ever
>>> get accepted and it's time to cut our losses.  (Patches that seem to
>>> be moving along in good order probably don't need any attention in
>>> this process, beyond determining that that's the case.)  That's why
>>> I think we need some senior people doing this, as their opinions are
>>> more likely to be taken seriously.
> 
>> Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
>> least move us forward? Granted it is less early and perhaps less often 
>> than the thread seems to indicate, but has been tossed around before and 
>> seems doable.
> 
> Perhaps.  The throughput of an N-person meeting is (at least) a factor
> of N less than the same N people looking at patches individually.
> On the other hand, the consensus of a meeting is more likely to be
> taken seriously than a single person's opinion, senior or not.
> So it could work, but I think we'd need some prefiltering so that
> the meeting only spends time on those patches already identified as
> needing help.
> 

I personally don't think the FOSDEM triage is a very productive use of
our time - we go through patches top to bottom, often with little idea
what the current state of the patch is. We always ran out of time after
looking at maybe 1/10 of the list.

Having an in-person discussion about patches would be good, but I think
we should split the meeting into much smaller groups for that, each
looking at a different subset. And maybe it should be determined in
advance, so that people can look at those patches in advance ...


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

Re: Conflict Detection and Resolution

[Tomas Vondra]

On 5/23/24 08:36, shveta malik wrote:
> Hello hackers,
> 
> Please find the proposal for Conflict Detection and Resolution (CDR)
> for Logical replication.
>  below details.>
> 
> Introduction
> 
> In case the node is subscribed to multiple providers, or when local
> writes happen on a subscriber, conflicts can arise for the incoming
> changes.  CDR is the mechanism to automatically detect and resolve
> these conflicts depending on the application and configurations.
> CDR is not applicable for the initial table sync. If locally, there
> exists conflicting data on the table, the table sync worker will fail.
> Please find the details on CDR in apply worker for INSERT, UPDATE and
> DELETE operations:
> 

Which architecture are you aiming for? Here you talk about multiple
providers, but the wiki page mentions active-active. I'm not sure how
much this matters, but it might.

Also, what kind of consistency you expect from this? Because none of
these simple conflict resolution methods can give you the regular
consistency models we're used to, AFAICS.

> INSERT
> 
> To resolve INSERT conflict on subscriber, it is important to find out
> the conflicting row (if any) before we attempt an insertion. The
> indexes or search preference for the same will be:
> First check for replica identity (RI) index.
>   - if not found, check for the primary key (PK) index.
> - if not found, then check for unique indexes (individual ones or
> added by unique constraints)
>  - if unique index also not found, skip CDR
> 
> Note: if no RI index, PK, or unique index is found but
> REPLICA_IDENTITY_FULL is defined, CDR will still be skipped.
> The reason being that even though a row can be identified with
> REPLICAT_IDENTITY_FULL, such tables are allowed to have duplicate
> rows. Hence, we should not go for conflict detection in such a case.
> 

It's not clear to me why would REPLICA_IDENTITY_FULL mean the table is
allowed to have duplicate values? It just means the upstream is sending
the whole original row, there can still be a PK/UNIQUE index on both the
publisher and subscriber.

> In case of replica identity ‘nothing’ and in absence of any suitable
> index (as defined above), CDR will be skipped for INSERT.
> 
> Conflict Type:
> 
> insert_exists: A conflict is detected when the table has the same
> value for a key column as the new value in the incoming row.
> 
> Conflict Resolution
> 
> a) latest_timestamp_wins:The change with later commit timestamp wins.
> b) earliest_timestamp_wins:   The change with earlier commit timestamp wins.
> c) apply:   Always apply the remote change.
> d) skip:Remote change is skipped.
> e) error:   Error out on conflict. Replication is stopped, manual
> action is needed.
> 

Why not to have some support for user-defined conflict resolution
methods, allowing to do more complex stuff (e.g. merging the rows in
some way, perhaps even with datatype-specific behavior)?

> The change will be converted to 'UPDATE' and applied if the decision
> is in favor of applying remote change.
> 
> It is important to have commit timestamp info available on subscriber
> when latest_timestamp_wins or earliest_timestamp_wins method is chosen
> as resolution method.  Thus ‘track_commit_timestamp’ must be enabled
> on subscriber, in absence of which, configuring the said
> timestamp-based resolution methods will result in error.
> 
> Note: If the user has chosen the latest or earliest_timestamp_wins,
> and the remote and local timestamps are the same, then it will go by
> system identifier. The change with a higher system identifier will
> win. This will ensure that the same change is picked on all the nodes.

How is this going to deal with the fact that commit LSN and timestamps
may not correlate perfectly? That is, commits may happen with LSN1 <
LSN2 but with T1 > T2.

> 
>  UPDATE
> 
> 
> Conflict Detection Method:
> 
> Origin conflict detection: The ‘origin’ info is used to detect
> conflict which can be obtained from commit-timestamp generated for
> incoming txn at the source node. To compare remote’s origin with the
> local’s origin, we must have origin information for local txns as well
> which can be obtained from commit-timestamp after enabling
> ‘track_commit_timestamp’ locally.
> The one drawback here is the ‘origin’ information cannot be obtained
> once the row is frozen and the commit-timestamp info is removed by
> vacuum. For a frozen row, conflicts cannot be raised, and thus the
> incoming changes will be applied in all the cases.
> 
> Conflict Types:
> 
> a) update_differ: The origin of an incoming update's key row differs
> from the local row i.e.; the row has already been updated locally or
> by different nodes.
> b) update_missing: The row with the same value as that incoming
> update's key does not exist. Remote is trying to update a row which
> does not exist l

Re: commitfest.postgresql.org is no longer fit for purpose

[Tom Lane]

Joe Conway  writes:
> On 5/24/24 15:45, Tom Lane wrote:
>> I was *not* proposing doing a regular review, unless of course
>> somebody really wants to do that.  What I am thinking about is
>> suggesting how to make progress on patches that are stuck, or in some
>> cases delivering the bad news that this patch seems unlikely to ever
>> get accepted and it's time to cut our losses.  (Patches that seem to
>> be moving along in good order probably don't need any attention in
>> this process, beyond determining that that's the case.)  That's why
>> I think we need some senior people doing this, as their opinions are
>> more likely to be taken seriously.

> Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
> least move us forward? Granted it is less early and perhaps less often 
> than the thread seems to indicate, but has been tossed around before and 
> seems doable.

Perhaps.  The throughput of an N-person meeting is (at least) a factor
of N less than the same N people looking at patches individually.
On the other hand, the consensus of a meeting is more likely to be
taken seriously than a single person's opinion, senior or not.
So it could work, but I think we'd need some prefiltering so that
the meeting only spends time on those patches already identified as
needing help.

regards, tom lane

Re: commitfest.postgresql.org is no longer fit for purpose

[Joe Conway]

On 5/24/24 15:45, Tom Lane wrote:

I was *not* proposing doing a regular review, unless of course
somebody really wants to do that.  What I am thinking about is
suggesting how to make progress on patches that are stuck, or in some
cases delivering the bad news that this patch seems unlikely to ever
get accepted and it's time to cut our losses.  (Patches that seem to
be moving along in good order probably don't need any attention in
this process, beyond determining that that's the case.)  That's why
I think we need some senior people doing this, as their opinions are
more likely to be taken seriously.


Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
least move us forward? Granted it is less early and perhaps less often 
than the thread seems to indicate, but has been tossed around before and 
seems doable.


--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Improving tracking/processing of buildfarm test failures

[Noah Misch]

On Thu, May 23, 2024 at 02:00:00PM +0300, Alexander Lakhin wrote:
> I'd like to discuss ways to improve the buildfarm experience for anyone who
> are interested in using information which buildfarm gives to us.
> 
> Unless I'm missing something, as of now there are no means to determine
> whether some concrete failure is known/investigated or fixed, how
> frequently it occurs and so on... From my experience, it's not that
> unbelievable that some failure occurred two years ago and lost in time was
> an indication of e. g. a race condition still existing in the code/tests
> and thus worth fixing. But without classifying/marking failures it's hard
> to find such or other interesting failure among many others...

I agree this is an area of difficulty consuming buildfarm results.  I have an
inefficient template for studying a failure, which your proposals would help:

 grep recent -hackers for animal name
 search the log for ~10 strings (e.g. "was terminated") to find the real 
indicator of where it failed
 search mailing lists for that indicator
 search buildfarm database for that indicator

> The first way to improve things I can imagine is to add two fields to the
> buildfarm database: a link to the failure discussion (set when the failure
> is investigated/reproduced and reported in -bugs or -hackers) and a commit
> id/link (set when the failure is fixed). I understand that it requires

I bet the hard part is getting data submissions, so I'd err on the side of
making this as easy as possible for submitters.  For example, accept free-form
text for quick notes, not only URLs and commit IDs.

> modifying the buildfarm code, and adding some UI to update these fields,
> but it allows to add filters to see only unknown/non-investigated failures
> in the buildfarm web interface later.
> 
> The second way is to create a wiki page, similar to "PostgreSQL 17 Open
> Items", say, "Known buildfarm test failures" and fill it like below:
> 
> 
> ...
> Useful info from the failure logs for reference
> ...
> 
> ---
> This way is less invasive, but it would work well only if most of
> interested people know of it/use it.
> (I could start with the second approach, if you don't mind, and we'll see
> how it works.)

Certainly you doing (2) can only help, though it may help less than (1).


I recommend considering what the buildfarm server could discover and publish
on its own.  Examples:

- N members failed at the same step, in a related commit range.  Those members
  are now mostly green.  Defect probably got fixed quickly.

- Log contains the following lines that are highly correlated with failure.
  The following other reports, if any, also contained them.

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Robert Haas  writes:
> On Fri, May 24, 2024 at 2:57 PM Tom Lane  wrote:
>> Doesn't seem right to me.  That will give pg_dump the wrong idea
>> of what the initial privileges actually were, and I don't see how
>> it can construct correct delta GRANT/REVOKE on the basis of false
>> information.  During the dump reload, the extension will be
>> recreated with the original owner (I think), causing its objects'
>> privileges to go back to the original pg_init_privs values.

> Oh! That does seem like it would make what I said wrong, but how would
> it even know who the original owner was? Shouldn't we be recreating
> the object with the owner it had at dump time?

Keep in mind that the whole point here is for the pg_dump script to
just say "CREATE EXTENSION foo", not to mess with the individual
objects therein.  So the objects are (probably) going to be owned by
the user that issued CREATE EXTENSION.

In the original conception, that was the end of it: what you got for
the member objects was whatever state CREATE EXTENSION left behind.
The idea of pg_init_privs is to support dump/reload of subsequent
manual alterations of privileges for extension-created objects.
I'm not, at this point, 100% certain that that's a fully realizable
goal.  But I definitely think it's insane to expect that to work
without also tracking changes in the ownership of said objects.

Maybe forbidding ALTER OWNER on extension-owned objects isn't
such a bad idea?

regards, tom lane

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Alexander Lakhin]

Hello,

24.05.2024 22:29, Tom Lane wrote:

The partition_split test has unstable results, as shown at [1].
I suggest adding "ORDER BY conname" to the two queries shown
to fail there.  Better look at other queries in the test for
possible similar problems, too.


Yes, I've just reproduced it on an aarch64 device as follows:
echo "autovacuum_naptime = 1
autovacuum_vacuum_threshold = 1
autovacuum_analyze_threshold = 1
" > ~/temp.config
TEMP_CONFIG=~/temp.config TESTS="$(printf 'partition_split %.0s' `seq 100`)" 
make -s check-tests
...
ok 80    - partition_split   749 ms
not ok 81    - partition_split   728 ms
ok 82    - partition_split   732 ms

$ cat src/test/regress/regression.diffs
diff -U3 .../src/test/regress/expected/partition_split.out 
.../src/test/regress/results/partition_split.out
--- .../src/test/regress/expected/partition_split.out   2024-05-15 
17:15:57.171999830 +
+++ .../src/test/regress/results/partition_split.out    2024-05-24 
19:28:37.32749 +
@@ -625,8 +625,8 @@
 SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 
'sales_feb_mar_apr2022'::regclass::oid;

pg_get_constraintdef | conname | conkey
 
-+-+
- CHECK ((sales_amount > 1))  | 
sales_range_sales_amount_check  | {2}
  FOREIGN KEY (salesperson_id) REFERENCES salespeople(salesperson_id) | 
sales_range_salesperson_id_fkey | {1}
+ CHECK ((sales_amount > 1))  | 
sales_range_sales_amount_check  | {2}
 (2 rows)

 ALTER TABLE sales_range SPLIT PARTITION sales_feb_mar_apr2022 INTO

Best regards,
Alexander

Re: Serverside SNI support in libpq

[Cary Huang]

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

This is an interesting feature on PostgreSQL server side where it can swap the
certificate settings based on the incoming hostnames in SNI field in client
hello message.

I think this patch resonate with a patch I shared awhile ago
( https://commitfest.postgresql.org/48/4924/ ) that adds multiple certificate
support on the libpq client side while this patch adds multiple certificate
support on the server side. My patch allows user to supply multiple certs, keys,
sslpasswords in comma separated format and the libpq client will pick one that
matches the CA issuer names sent by the server. In relation with your patch,
this CA issuer name would match the CA certificate configured in pg_hosts.cfg.

I had a look at the patch and here's my comments:

+   
+PostgreSQL can be configured for
+SNI using the pg_hosts.conf
+configuration file. PostgreSQL inspects the TLS
+hostname extension in the SSL connection handshake, and selects the right
+SSL certificate, key and CA certificate to use for the connection.
+   

pg_hosts should also have sslpassword_command just like in the postgresql.conf 
in
case the sslkey for a particular host is encrypted with a different password.

+   /*
+* Install SNI TLS extension callback in case the server is configured 
to
+* validate hostnames.
+*/
+   if (ssl_snimode != SSL_SNIMODE_OFF)
+   SSL_CTX_set_tlsext_servername_callback(context, 
sni_servername_cb);

If libpq client does not provide SNI, this callback will not be called, so there
is not a chance to check for a hostname match from pg_hosts, swap the TLS 
CONTEXT,
or possibly reject the connection even in strict mode. The TLS handshake in such
case shall proceed and server will use the certificate specified in
postgresql.conf (if these are loaded) to complete the handshake with the client.
There is a comment in the patch that reads:

>  - strict: only pg_hosts.conf is loaded and the TLS extension hostname
>   MUST be passed and MUST have a match in the configuration, else the
>  connection is refused.

I am not sure if it implies that if ssl_snimode is strict, then the normal 
ssl_cert,
ssl_key and ca_cert…etc settings in postgresql.conf are ignored?

thank you

Cary Huang
-
HighGo Software Inc. (Canada)
cary.hu...@highgo.ca
www.highgo.ca

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Robert Haas]

On Fri, May 24, 2024 at 2:57 PM Tom Lane  wrote:
> Doesn't seem right to me.  That will give pg_dump the wrong idea
> of what the initial privileges actually were, and I don't see how
> it can construct correct delta GRANT/REVOKE on the basis of false
> information.  During the dump reload, the extension will be
> recreated with the original owner (I think), causing its objects'
> privileges to go back to the original pg_init_privs values.

Oh! That does seem like it would make what I said wrong, but how would
it even know who the original owner was? Shouldn't we be recreating
the object with the owner it had at dump time?

> Although ... this is tickling a recollection that pg_dump doesn't
> try very hard to run CREATE EXTENSION with the same owner that
> the extension had originally.  That's a leftover from the times
> when basically all extensions required superuser to install,
> and of course one superuser is as good as the next.  There might
> be some work we have to do on that side too if we want to up
> our game in this area.

Hmm, yeah.

> Another case that's likely not handled well is what if the extension
> really shouldn't have its original owner (e.g. you're using
> --no-owner).  If it's restored under a new owner then the
> pg_init_privs data certainly doesn't apply, and it feels like it'd
> be mostly luck if the precomputed delta GRANT/REVOKEs lead to a
> state you like.

I'm not sure exactly how this computation works, but if tgl granted
nmisch privileges on an object and the extension is now owned by
rhaas, it would seem like the right thing to do would be for rhaas to
grant nmisch those same privileges. Conversely if tgl started with
privileges to do X and Y and later was granted privileges to do Z and
we dump and restore such that the extension is owned by rhaas, I'd
presume rhaas would end up with those same privileges. I'm probably
too far from the code to give terribly useful advice here, but I think
the expected behavior is that the new owner replaces the old one for
all purposes relating to the owned object(s). At least, I can't
currently see what else makes any sense.

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

Re: commitfest.postgresql.org is no longer fit for purpose

[Tom Lane]

Tomas Vondra  writes:
> On 5/24/24 19:09, Tom Lane wrote:
 ... Maybe we could divide and conquer: get a
 dozen-or-so senior contributors to split up the list of pending
 patches and then look at each one with an eye to what needs to
 happen to move it along (*not* to commit it right away, although
 in some cases maybe that's the thing to do).

> I think meeting all these conditions - a week early in the cycle, but
> not in the summer, when everyone can focus on this - will be difficult.

True.  Perhaps in the fall there'd be a better chance?

> So maybe it'd be better to just set some deadline by which this needs to
> be done, and make sure every pending patch has someone expected to look
> at it? IMHO we're not in position to assign stuff to people, so I guess
> people would just volunteer anyway - the CF app might track this.

One problem with a time-extended process is that the set of CF entries
is not static, so a predetermined division of labor will result in
missing some newly-arrived entries.  Maybe that's not a problem
though; anything newly-arrived is by definition not "stuck".  But we
would definitely need some support for keeping track of what's been
looked at and what remains, whereas if it happens over just a few
days that's probably not so essential.

> It's not entirely clear to me if this would effectively mean doing a
> regular review of those patches, or something less time consuming.

I was *not* proposing doing a regular review, unless of course
somebody really wants to do that.  What I am thinking about is
suggesting how to make progress on patches that are stuck, or in some
cases delivering the bad news that this patch seems unlikely to ever
get accepted and it's time to cut our losses.  (Patches that seem to
be moving along in good order probably don't need any attention in
this process, beyond determining that that's the case.)  That's why
I think we need some senior people doing this, as their opinions are
more likely to be taken seriously.

regards, tom lane

Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

[Tom Lane]

The partition_split test has unstable results, as shown at [1].
I suggest adding "ORDER BY conname" to the two queries shown
to fail there.  Better look at other queries in the test for
possible similar problems, too.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jackdaw&dt=2024-05-24%2015%3A58%3A17

Re: commitfest.postgresql.org is no longer fit for purpose

[Tomas Vondra]

On 5/24/24 19:09, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 5/20/24 16:16, Robert Haas wrote:
>>> On Sun, May 19, 2024 at 3:18 PM Tom Lane  wrote:
 * Before starting this thread, Robert did a lot of very valuable
 review of some individual patches.  I think what prompted him to
 start the thread was the realization that he'd only made a small
 dent in the problem.  Maybe we could divide and conquer: get a
 dozen-or-so senior contributors to split up the list of pending
 patches and then look at each one with an eye to what needs to
 happen to move it along (*not* to commit it right away, although
 in some cases maybe that's the thing to do).  It'd be great if
 that could happen just before each commitfest, but that's probably
 not practical with the current patch volume.  What I'm thinking
 for the moment is to try to make that happen once a year or so.
> 
>>> I like this idea.
> 
>> Me too. Do you think it'd happen throughout the whole year, or at some
>> particular moment?
> 
> I was imagining a focused community effort spanning a few days to
> a week.  It seems more likely to actually happen if we attack it
> that way than if it's spread out as something people will do when
> they get around to it.  Of course the downside is finding a week
> when everybody is available.
> 
>> We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
>> be more of an ad hoc thing to use the remaining time, with only a small
>> subset of people. But that seems pretty late in the dev cycle, I guess
>> we'd want it to happen early, perhaps during the first CF?
> 
> Yeah, early in the cycle seems more useful, although the summer might
> be the worst time for peoples' availability.
> 

I think meeting all these conditions - a week early in the cycle, but
not in the summer, when everyone can focus on this - will be difficult.

If we give up on everyone doing it at the same time, summer would be a
good time to do this - it's early in the cycle, and it tends to be a
quieter period (customers are on vacation too, so fewer incidents).

So maybe it'd be better to just set some deadline by which this needs to
be done, and make sure every pending patch has someone expected to look
at it? IMHO we're not in position to assign stuff to people, so I guess
people would just volunteer anyway - the CF app might track this.

It's not entirely clear to me if this would effectively mean doing a
regular review of those patches, or something less time consuming.


regards

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

Re: Add minimal C example and SQL registration example for custom table access methods.

[Fabrízio de Royes Mello]

On Fri, May 24, 2024 at 3:11 PM Phil Eaton  wrote:

> > I think this should say something more like "Here is how an extension
> > SQL script might create a table access method handler".
>
> Fair point. It is referred to elsewhere [0] in docs as a "script
> file", so I've done that.
>
> > Shouldn't "mem_tableam_handler" be "my_tableam_handler"?
>
> Sorry about that, fixed.
>
> [0] https://www.postgresql.org/docs/current/extend-extensions.html
>
> Phil
>

Nice... LGTM!

-- 
Fabrízio de Royes Mello

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Robert Haas  writes:
> On Fri, May 24, 2024 at 11:59 AM Tom Lane  wrote:
>> So this goal seems to
>> mean that neither ALTER OWNER nor REASSIGN OWNED should touch
>> pg_init_privs at all, as that would break its function of recording
>> a historical state.  Only DROP OWNED should get rid of pg_init_privs
>> grants, and that only because there's no choice -- if the role is
>> about to go away, we can't hang on to a reference to its OID.

> But I would have thought that the right thing to do to pg_init_privs
> here would be essentially s/$OLDOWNER/$NEWOWNER/g.

Doesn't seem right to me.  That will give pg_dump the wrong idea
of what the initial privileges actually were, and I don't see how
it can construct correct delta GRANT/REVOKE on the basis of false
information.  During the dump reload, the extension will be
recreated with the original owner (I think), causing its objects'
privileges to go back to the original pg_init_privs values.
Applying a delta that starts from some other state seems pretty
questionable in that case.

It could be that if we expect pg_dump to issue an ALTER OWNER
to move ownership of the altered extension object to its new
owner, and only then apply its computed delta GRANT/REVOKEs,
then indeed the right thing is for the original ALTER OWNER
to apply s/$OLDOWNER/$NEWOWNER/g to pg_init_privs.  I've not
thought this through in complete detail, but it feels like
that might work, because the reload-time ALTER OWNER would
apply exactly that change to both the object's ACL and its
pg_init_privs, and then the delta is starting from the right state.
Of course, pg_dump can't do that right now because it lacks the
information that such an ALTER is needed.

Although ... this is tickling a recollection that pg_dump doesn't
try very hard to run CREATE EXTENSION with the same owner that
the extension had originally.  That's a leftover from the times
when basically all extensions required superuser to install,
and of course one superuser is as good as the next.  There might
be some work we have to do on that side too if we want to up
our game in this area.

Another case that's likely not handled well is what if the extension
really shouldn't have its original owner (e.g. you're using
--no-owner).  If it's restored under a new owner then the
pg_init_privs data certainly doesn't apply, and it feels like it'd
be mostly luck if the precomputed delta GRANT/REVOKEs lead to a
state you like.

regards, tom lane

Document use of ldapurl with LDAP simple bind

[Jacob Champion]

Hi all,

Our documentation implies that the ldapurl setting in pg_hba is used
for search+bind mode only. It was pointed out to me recently that this
is not true, and if you're dealing with simple bind on a non-standard
scheme or port, then ldapurl makes the HBA easier to read:

... ldap ldapurl="ldaps://ldap.example.net:49151" ldapprefix="cn="
ldapsuffix=", dc=example, dc=net"

0001 tries to document this helpful behavior a little better, and 0002
pins it with a test. WDYT?

Thanks,
--Jacob


0001-docs-explain-how-to-use-ldapurl-with-simple-bind.patch
Description: Binary data


0002-ldap-test-ldapurl-with-simple-bind.patch
Description: Binary data

Re: apply_scanjoin_target_to_paths and partitionwise join

[Robert Haas]

On Fri, May 24, 2024 at 2:02 PM  wrote:
> I am not sure, whether it's really a bug. I personally wouldn't be brave
> enough to back patch this. I don't want to deal with complaining end
> users. Suddenly their optimizer, which always had horrible estimates,
> was actually able to do harmful stuff with them. Only due to a minor
> version upgrade. I think that's a bad idea to backpatch something with
> complex performance implications. Especially since they might even be
> based on potentially inaccurate data...

+1.

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

Re: First draft of PG 17 release notes

[Peter Geoghegan]

On Fri, May 24, 2024 at 1:50 PM Andres Freund  wrote:
> Bruce, just about everyone seems to disagree with your current approach. And
> not just this year, this has been a discussion in most if not all release note
> threads of the last few years.

+1.

> People, including me, *have* addressed your criteria, but you just waved those
> concerns away. It's hard to continue discussing criteria when it doesn't at
> all feel like a conversation.

At one point on this thread, Bruce said "I am particularly critical if
I start to wonder, "Why does the author _think_ I should care about
this?" because it feels like the author is writing for him/herself and
not the audience."

Whenever this sort of thing has come up in the past, and I pushed
back, Bruce seemed to respond along these lines: he seemed to suggest
that there was some kind of conflict of interests involved. This isn't
completely unreasonable, of course -- my motivations aren't wholly
irrelevant. But for the most part they're *not* very relevant, and
wouldn't be even if Bruce's worst suspicions were actually true. In
principle it shouldn't matter that I'm biased, if I happen to be
correct in some relevant sense.

Everybody has some kind of bias. Even if my bias in these matters was
a significant factor (which I tend to doubt), I don't think that it's
fair to suggest that it's self-serving or careerist. My bias was
probably present before I even began work on the feature in question.
I tend to work on things because I believe that they're important --
it's not the other way around (at least not to a significant degree).

> In the end, these are patches to the source code, I don't think you can just
> wave away widespread disagreement with your changes. That's not how we do
> postgres development.

In lots of cases (a large minority of cases) the problem isn't even
really with the emphasis of one type of item over another/the
inclusion or non-inclusion of some individual item. It's actually a
problem with the information being presented in the most useful way.

Often I've suggested what I believe to be a better wording on the
merits (usually less obscure and more accessible language), only to be
met with the same sort of resistance from Bruce. If I've put a huge
amount of work into the item (as is usually the case), then I think
that I am at least entitled to a fair hearing.

I don't expect Bruce to meet me halfway, or even for him to meet me a
quarter of the way -- somebody has to be empowered to say no here
(even to very senior community members). I just don't think that he
has seriously considered my feedback in this area over the years. Not
always, not consistently, but often enough for it to seem like a real
problem.

--
Peter Geoghegan

Re: First draft of PG 17 release notes

[Andres Freund]

Hi,

On 2024-05-22 18:33:03 -0400, Bruce Momjian wrote:
> On Tue, May 21, 2024 at 09:40:28AM -0700, Andres Freund wrote:
> > On 2024-05-18 11:13:54 -0400, Bruce Momjian wrote:
> > I agree keeping things reasonably short is important. But I don't think 
> > you're
> > evenly applying it as a goal.
> >
> > Just skimming the notes from the end, I see
> > - an 8 entries long pg_stat_statements section
>
> What item did you want to remove?  Those are all user-visible changes.

My point here was not that we necessarily need to remove those, but that their
impact to users is smaller than many of the performance impacts you disregard.


> > - multiple entries about "Create custom wait events for ..."
>
> Well, those are all in different sections, so how can they be merged,
> unless I create a "wait event section", I guess.

They're not, all are in "Additional Modules". Instead of

- Create custom wait events for postgres_fdw (Masahiro Ikeda)
- Create custom wait events for dblink (Masahiro Ikeda)
- Allow extensions to define custom wait events (Masahiro Ikeda)

I'd make it:

- Allow extensions to define custom wait events and create custom wait events
  for postgres_fdw, dblink (Masahiro Ikeda)


> > - three entries about adding --all to {reindexdb,vacuumdb,clusterdb}.
>
> The problem with merging these is that the "Specifically, --all can now
> be used with" is different for all three of them.

You said you were worried about the length of the release notes, because it
discourages users from actually reading the release notes, due to getting
bored. Having three instance of almost the same entry, with just minor changes
between them, seems to precisely endanger boring readers.

I'd probably just go for

- Add --all option to clusterdb, reindexdb, vacuumdb to process objects in all
  databases matching a pattern (Nathan Bossart)

or such. The precise details of how the option works for the different
commands doesn't need to be stated in the release notes, that's more of a
reference documentation thing. But if you want to include it, we can do
something like

  Specifically, --all can now be used with --table (all commands), --schema
  (reindexdb, vacuumdb), and --exclude-schema (reindexdb, vacuumdb).


> > - an entry about adding long options to pg_archivecleanup
>
> Well, that is a user-visible change.  Should it not be listed?

If you are concerned about the length of the release notes and as a
consequence not including more impactful performance changes, then no, it
shouldn't. It doesn't break anyones current scripts, it doesn't enable
anything new.


> > - two entries about grantable maintenance rights, once via pg_maintain, once
> >   per-table
>
> Well, one is a GRANT and another is a role, so merging them seemed like
> it would be too confusing.

I don't think it has to be.

Maybe something roughly like

- Allow granting the right to perform maintenance operations (Nathan Bossart)

  The permission can be granted on a per-table basis using the MAINTAIN
  privilege and on a system wide basis via the pg_maintain role.

  Operations that can be controlled are VACUUM, ANALYZE, REINDEX, REFRESH
  MATERIALIZED VIEW, CLUSTER, and LOCK TABLE.


I'm again mostly reacting to your concern that the release notes are getting
too boring to read. Repeated content, like in the current formulation, imo
does endanger that. Current it is:

- Add per-table GRANT permission MAINTAIN to control maintenance operations 
(Nathan Bossart)

  The operations are VACUUM, ANALYZE, REINDEX, REFRESH MATERIALIZED VIEW, 
CLUSTER, and LOCK TABLE.

- Add user-grantable role pg_maintain to control maintenance operations (Nathan 
Bossart)

  The operations are VACUUM, ANALYZE, REINDEX, REFRESH MATERIALIZED VIEW, 
CLUSTER, and LOCK TABLE.



> > - separate entries about pg_stat_reset_slru(), pg_stat_reset_shared("slru"),
>
> They are different functions with different detail text.

So what? You can change their text. Making it three entries makes it harder
for a reader that doesn't care about resetting stats to skip over the details.

Make it something like

- Improve control over resetting statistics (Atsushi Torikoshi, Bharath
  Rupireddy)

  pg_stat_reset_shared() can now reset all shared statistics, by passing NULL;
  pg_stat_reset_shared(NULL) also resets SLRU statistics;
  pg_stat_reset_shared("slru") resets SLRU statistics, which was already
  possible using pg_stat_reset_slru(NULL).

Greetings,

Andres Freund

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Robert Haas]

On Fri, May 24, 2024 at 11:59 AM Tom Lane  wrote:
> Thinking about this some more: the point of pg_init_privs is to record
> an object's privileges as they stood at the end of CREATE EXTENSION
> (or extension update), with the goal that pg_dump should be able to
> compute the delta between that and the object's current privileges
> and emit GRANT/REVOKE commands to restore those current privileges
> after a fresh extension install.  (We slide gently past the question
> of whether the fresh extension install is certain to create privileges
> matching the previous pg_init_privs entry.)

+1 to all of this.

> So this goal seems to
> mean that neither ALTER OWNER nor REASSIGN OWNED should touch
> pg_init_privs at all, as that would break its function of recording
> a historical state.  Only DROP OWNED should get rid of pg_init_privs
> grants, and that only because there's no choice -- if the role is
> about to go away, we can't hang on to a reference to its OID.

But I would have thought that the right thing to do to pg_init_privs
here would be essentially s/$OLDOWNER/$NEWOWNER/g.

I know I'm late to the party here, but why is that idea wrong?

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

Re: Add minimal C example and SQL registration example for custom table access methods.

[Phil Eaton]

> I think this should say something more like "Here is how an extension
> SQL script might create a table access method handler".

Fair point. It is referred to elsewhere [0] in docs as a "script
file", so I've done that.

> Shouldn't "mem_tableam_handler" be "my_tableam_handler"?

Sorry about that, fixed.

[0] https://www.postgresql.org/docs/current/extend-extensions.html

Phil


v4-0001-Add-minimal-C-example-and-SQL-registration-exampl.patch
Description: Binary data

Re: apply_scanjoin_target_to_paths and partitionwise join

[arne . roland]

Hi Ashutosh,

thanks for bringing this to my attention. I'll first share a few 
thoughts about the change and respond regarding the test below.


I clearly understand your intention with this patch. It's an issue I run 
into from time to time.


I did some testing with some benchmark sets back with pg 14. I did the 
following: I planned with and without the partitionwise join GUC 
(explain) and took the one with the lower cost to execute the query.


Interestingly, even discounting the overhead and additional planning 
time, the option with the lower cost turned out to be slower on our 
benchmark set back then. The median query with disabled GUC was quicker, 
but on average that was not the case. The observation is one, I'd 
generally describe as "The more options you consider, the more ways we 
have to be horribly wrong. More options for the planner are a great way 
to uncover the various shortcomings of it."


That might be specific to the benchmark I was working with at the time. 
But that made me drop the issue back then. That is ofc no valid reason 
not to go in the direction of making the planner to consider more 
options. :)


Maybe we can discuss that in person next week?

On 2024-05-22 07:57, Ashutosh Bapat wrote:

On Mon, May 6, 2024 at 6:28 PM Ashutosh Bapat
 wrote:

4. meson test ends up with failures like below:

4/290 postgresql:regress / regress/regress
ERROR  32.67s
6/290 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade
ERROR  56.96s
35/290 postgresql:recovery / recovery/027_stream_regress
ERROR  40.20s

(all due to "regression tests pass" failures)
[...]



So with the patch that SQL does not use partitionwise join as it
finds
it more optimal to stick to a plan with cost of "1.10..2.21"
instead
of "1.11..2.22" (w/ partition_join), nitpicking but still a
failure
technically. Perhaps it could be even removed? (it's pretty close
to
noise?).


The test was added by 6b94e7a6da2f1c6df1a42efe64251f32a444d174 and
later modified by 3c569049b7b502bb4952483d19ce622ff0af5fd6. The
modification just avoided eliminating the join, so that change can be
ignored. 6b94e7a6da2f1c6df1a42efe64251f32a444d174 added the tests to
test fractional paths being considered when creating ordered append
paths. Reading the commit message, I was expecting a test which did
not use a join as well and also which used inheritance. But it seems
that the queries added by that commit, test all the required scenarios
and hence we see two queries involving join between partitioned
tables. As the comment there says the intention is to verify index
only scans and not exactly partitionwise join. So just fixing the
expected output of one query looks fine. The other query will test
partitionwise join and fractional paths anyway. I am including Tomas,
Arne and Zhihong, who worked on the first commit, to comment on
expected output changes.


The test was put there to make sure a fractional join is considered in 
the case that a partitionwise join is considered. Because that wasn't 
the case before.


The important part for my use case back then was that we do Merge 
Join(s) at all. The test result after your patch still confirms that.


If we simply modify the test as such, we no longer confirm, whether the 
code path introduced in 6b94e7a6da2f1c6df1a42efe64251f32a444d174 is 
still working.


Maybe it's worthwhile to add something like

create index on fract_t0 ((id*id));

EXPLAIN (COSTS OFF)
SELECT * FROM fract_t x JOIN fract_t y USING (id) ORDER BY id * id DESC 
LIMIT 10;

  QUERY PLAN
---
 Limit
   ->  Merge Append
 Sort Key: ((x.id * x.id)) DESC
 ->  Nested Loop
   ->  Index Scan Backward using fract_t0_expr_idx on 
fract_t0 x_1

   ->  Index Only Scan using fract_t0_pkey on fract_t0 y_1
 Index Cond: (id = x_1.id)
 ->  Sort
   Sort Key: ((x_2.id * x_2.id)) DESC
   ->  Hash Join
 Hash Cond: (x_2.id = y_2.id)
 ->  Seq Scan on fract_t1 x_2
 ->  Hash
   ->  Seq Scan on fract_t1 y_2


I am not sure, whether it's worth the extra test cycles on every animal, 
but since we are not creating an extra table it might be ok.

I don't have a very strong feeling about the above test case.


I will create patches for the back-branches once the patch for master
is in a committable state.


I am not sure, whether it's really a bug. I personally wouldn't be brave 
enough to back patch this. I don't want to deal with complaining end 
users. Suddenly their optimizer, which always had horrible estimates, 
was actually able to do harmful stuff with them. Only due to a minor 
version upgrade. I think that's a bad idea to backpatch something with 
complex performance implications. Especially since they might even be 
based on potentially inaccurate data...

Re: First draft of PG 17 release notes

[Andres Freund]

Hi,

On 2024-05-23 23:27:04 -0400, Bruce Momjian wrote:
> On Thu, May 23, 2024 at 11:11:10PM -0400, Tom Lane wrote:
> > Bruce Momjian  writes:
> > I am not sure Bruce that you realize that your disregard for
> > performance improvements is shared by nobody.  Arguably,
> > performance is 90% of what we do these days, and it's also
> > 90% of what users care about.
>
> Please stop saying I don't document performance.  I have already
> explained enough which performance items I choose.  Please address my
> criteria or suggest new criteria.

Bruce, just about everyone seems to disagree with your current approach. And
not just this year, this has been a discussion in most if not all release note
threads of the last few years.

People, including me, *have* addressed your criteria, but you just waved those
concerns away. It's hard to continue discussing criteria when it doesn't at
all feel like a conversation.

In the end, these are patches to the source code, I don't think you can just
wave away widespread disagreement with your changes. That's not how we do
postgres development.

Greetings,

Andres Freund

Re: PG17beta1: Unable to test Postgres on Fedora due to fatal Error in psql: undefined symbol: PQsocketPoll

[Tom Lane]

Hans Buschmann  writes:
> When I tried to connect to the restored database with psql \c I got:
> ...
> postgres=# \c cpsdb
> pgbeta/bin/psql: symbol lookup error: pgbeta/bin/psql: undefined symbol: 
> PQsocketPoll

> (To my understanding) the problem comes from incompatible libpq.so libraries 
> on the system.

Right, you must have a v16-or-earlier libpq lying around somewhere,
and psql has bound to that not to the beta-test version.
PQsocketPoll is new in v17.

> - Why doesn't psql use the just created lib64/libpq.so.5.17 from ninja 
> install?

It's on you to ensure that happens, especially on Linux systems which
have a strong bias towards pulling libraries from /usr/lib[64].
Normally our --enable-rpath option is sufficient; while that's
default in an autoconf-based build, I'm not sure that it is
in a meson build.  Also, if your beta libpq is not where the
rpath option expected it to get installed, the linker will silently
fall back to /usr/lib[64].

> The loading of the locally available libpq.so should always have priority 
> over a system wide in /usr/lib64

Tell it to the Linux developers --- they think the opposite.
Likewise, all of your other proposals need to be addressed to
the various distros' packagers; this is not the place to complain.

The main thing that is bothering me about the behavior you
describe is that it didn't fail until psql actually tried to
call PQsocketPoll.  (AFAICT from a quick look, that occurs
during \c but not during the startup connection.)  I had thought
that we select link options that result in early binding and
hence startup-time failure for a case like this.  I can confirm
though that this acts as described on my RHEL8 box if I force
current psql to link to v16 libpq, so either we've broken that
or it never did apply to frontend programs.  But it doesn't
seem to me to be a great thing for it to behave like this.
You could easily miss that you have a broken setup until
after you deploy it.

regards, tom lane

Re: Upgrade Debian CI images to Bookworm

[Andres Freund]

Hi,

On 2024-05-24 16:17:37 +0200, Peter Eisentraut wrote:
> I'm not sure what the backpatching expectations of this kind of thing is.
> The history of this CI setup is relatively short, so this hasn't been
> stressed too much.  I see that we once backpatched the macOS update, but
> that might have been all.

I've backpatched a few other changes too.


> If we start backpatching this kind of thing, then this will grow as a job
> over time.  We'll have 5 or 6 branches to keep up to date, with several
> operating systems.  And once in a while we'll have to make additional
> changes like this warning fix you mention here.  I'm not sure how much we
> want to take this on.  Is there ongoing value in the CI setup in
> backbranches?

I find it extremely useful to run CI on backbranches before
batckpatching. Enough so that I've thought about proposing backpatching CI all
the way.

I don't think it's that much work to fix this kind of thing in the
backbranches. We don't need to backpatch new tasks or such. Just enough stuff
to keep e.g. the base image the same - otherwise we end up running CI on
unsupported distros, which doesn't help anybody.


> With these patches, we could do either of the following:
> 5) We update master, PG16, and PG15, but we hold all of them until the
> warning in PG15 is fixed.

I think we should apply the fix in <= 15 - IMO it's a correct compiler
warning, what we do right now is wrong.

Greetings,

Andres Freund

Re: warn if GUC set to an invalid shared library

[Robert Haas]

On Fri, May 24, 2024 at 11:48 AM Justin Pryzby  wrote:
> You give me too much credit..

Gee, usually I'm very good at avoiding that mistake. :-)

> We don't want to change the behavior to allow this to succeed -- it
> would allow leaving the server in a state that it fails to start (rather
> than helping to avoid doing so, as intended by this thread).

+1.

> Maybe there should be a comment explaning why PGC_FILE is used, and
> maybe there should be a TAP test for the behavior, but they're pretty
> unrelated to this thread.  So, I've dropped the 001 patch.

+1 for that, too.

+ /* Note that filename was already canonicalized */

I see that this comment is copied from load_libraries(), but I don't
immediately see where the canonicalization actually happens. Do you
know, or can you find out? Because that's crucial here, else stat()
might not target the real filename. I wonder if it will anyway. Like,
couldn't the library be versioned, and might not dlopen() try a few
possibilities?

+ errdetail("The server will currently fail to start with this setting."),
+ errdetail("New sessions will currently fail to connect with the new
setting."));

I understand why these messages have the word "currently" in them, but
I bet the user won't. I'm not sure exactly what to recommend at the
moment (and I'm quite busy today due to the conference upcoming) but I
think we should try to find some way to rephrase these.

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

Re: commitfest.postgresql.org is no longer fit for purpose

[Tom Lane]

Tomas Vondra  writes:
> On 5/20/24 16:16, Robert Haas wrote:
>> On Sun, May 19, 2024 at 3:18 PM Tom Lane  wrote:
>>> * Before starting this thread, Robert did a lot of very valuable
>>> review of some individual patches.  I think what prompted him to
>>> start the thread was the realization that he'd only made a small
>>> dent in the problem.  Maybe we could divide and conquer: get a
>>> dozen-or-so senior contributors to split up the list of pending
>>> patches and then look at each one with an eye to what needs to
>>> happen to move it along (*not* to commit it right away, although
>>> in some cases maybe that's the thing to do).  It'd be great if
>>> that could happen just before each commitfest, but that's probably
>>> not practical with the current patch volume.  What I'm thinking
>>> for the moment is to try to make that happen once a year or so.

>> I like this idea.

> Me too. Do you think it'd happen throughout the whole year, or at some
> particular moment?

I was imagining a focused community effort spanning a few days to
a week.  It seems more likely to actually happen if we attack it
that way than if it's spread out as something people will do when
they get around to it.  Of course the downside is finding a week
when everybody is available.

> We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
> be more of an ad hoc thing to use the remaining time, with only a small
> subset of people. But that seems pretty late in the dev cycle, I guess
> we'd want it to happen early, perhaps during the first CF?

Yeah, early in the cycle seems more useful, although the summer might
be the worst time for peoples' availability.

regards, tom lane

PG17beta1: Unable to test Postgres on Fedora due to fatal Error in psql: undefined symbol: PQsocketPoll

[Hans Buschmann]

  1.  Problem and reproducability


After the release of  PG17b1 I wanted to test it on a newly installed machine:

I installed Fedora 40 Server on x86-64 and did a full dnf update (as of 23.may 
2024).


To self-compile from source I did:


sudo dnf group install "C Development*" "Development*"

sudo dnf install wine mold meson perl zstd lz4 lz4-devel libzstd-devel
(This may be a little too much, I was lazy for minimal installation)

I built Postgres 17 beta1 from source with meson and installed it with ninja 
install.
I started the server and created a new database with psql
I restored a test database from dump with pg_restore.

When I tried to connect to the restored database with psql \c I got:


[root@localhost local]# sudo -u postgres pgbeta/bin/psql -h /tmp -p 5431
psql (17beta1)
Type "help" for help.

postgres=# select version ();
  version

 PostgreSQL 17beta1 on x86_64-linux, compiled by gcc-14.1.1, 64-bit
(1 row)

postgres=# \l
List of databases
   Name|  Owner   | Encoding | Locale Provider | Collate | Ctype | Locale | 
ICU Rules |   Access privileges
---+--+--+-+-+---++---+---
 cpsdb | postgres | UTF8 | builtin | C   | C | C  | 
  |
 postgres  | postgres | UTF8 | builtin | C   | C | C  | 
  |
 template0 | postgres | UTF8 | builtin | C   | C | C  | 
  | =c/postgres  +
   |  |  | | |   || 
  | postgres=CTc/postgres
 template1 | postgres | UTF8 | builtin | C   | C | C  | 
  | =c/postgres  +
   |  |  | | |   || 
  | postgres=CTc/postgres
(4 rows)

postgres=# \c cpsdb
pgbeta/bin/psql: symbol lookup error: pgbeta/bin/psql: undefined symbol: 
PQsocketPoll
[root@localhost local]#

--- ERROR 
^^

So it was not possible to use the database locally with psql.

2. Analysis

(To my understanding) the problem comes from incompatible libpq.so libraries on 
the system.
The installation of all the development packages installed :

[root@localhost lib64]# dnf list installed libpq
Installed Packages
libpq.x86_64 16.1-4.fc40
  @fedora

This is the older version provided by Fedora (Nov 2023!, even after 16.3 from 
May 2024)

3. Questions

- Why doesn't psql use the just created lib64/libpq.so.5.17 from ninja install?

The loading of the locally available libpq.so should always have priority over 
a system wide in /usr/lib64

- Why is the Fedora supplied library 2 minor versions behind?

- How to install the new libpq systemwide to make it usable for all 
applications (not only postgres supplied clients)?
To my understanding libpq is normally downward compatible, so it may be 
possible to use libpq17 against all supported older releases

4. Proposals

- The distributors like Fedora, Debian,Ubuntu etc. should be encouraged to 
update the minor versions IN A TIMELY FASHION like any other upgrades: Minor 
versions normally don't break anything and often contain security fixes and 
important bug fixes valuable to all update-willing users of the system. Perhaps 
somebody deeper involved can support the distributors in this case.

-  PGDG should provide the newest libpq.so (beta or stable) in its common 
repository starting at the first beta release of a major version.
So everybody can install it separately and test it against its own application. 
This should ease real world testing alot.

- Due to the downward compatibility of libpq and the difficulty of handling 
multiple versions on the same machine I propose to always provide the newest 
libpq (highest stable version and latest beta version) for separate 
installation.
This should be independend installable from the main packages, much like the 
Visual runtime libraries for applications under Windows.

The user can choose between the latest stable version (at this time libpq 
16.3), the latest beta (at this time libpq 17beta) or the version belonging to 
its major/minor version. This should be documented and be easyly changeable.

The buildfarm could be run always with the most current version. libpq could be 
thought of a separate product as base for all client applications.

5. Solving the problem

I don't know how to solve the problem in an official way.

I haven't tried manual changes in the system (copying binaries, changing 
symbolic links, making own packages etc.)

I am able to access the test database from outside (from a windows psql client 
of pg17b1), but this is not very practic

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Daniel Gustafsson  writes:
> On 24 May 2024, at 16:20, Tom Lane  wrote:
>> Another point: shdepReassignOwned explicitly does not touch grants
>> or default ACLs.  It feels like the same should be true of
>> pg_init_privs entries,

> Agreed, I can't see why pg_init_privs should be treated differently.

Thinking about this some more: the point of pg_init_privs is to record
an object's privileges as they stood at the end of CREATE EXTENSION
(or extension update), with the goal that pg_dump should be able to
compute the delta between that and the object's current privileges
and emit GRANT/REVOKE commands to restore those current privileges
after a fresh extension install.  (We slide gently past the question
of whether the fresh extension install is certain to create privileges
matching the previous pg_init_privs entry.)  So this goal seems to
mean that neither ALTER OWNER nor REASSIGN OWNED should touch
pg_init_privs at all, as that would break its function of recording
a historical state.  Only DROP OWNED should get rid of pg_init_privs
grants, and that only because there's no choice -- if the role is
about to go away, we can't hang on to a reference to its OID.

However ... then what are the implications of doing ALTER OWNER on
an extension-owned object?  Is pg_dump supposed to recognize that
that's happened and replay it too?  If not, is it sane at all to
try to restore the current privileges, which are surely dependent
on the current owner?  I kind of doubt that that's possible at all,
and even if it is it might result in security issues.  It seems
like pg_init_privs has missed a critical thing, which is to record
the original owner not only the original privileges.

(Alternatively, maybe we should forbid ALTER OWNER on extension-owned
objects?  Or at least on those having pg_init_privs entries?)


I'm wondering too about this scenario:

1. CREATE EXTENSION installs an object and sets some initial privileges.

2. DBA manually modifies the object's privileges.

3. ALTER EXTENSION UPDATE further modifies the object's privileges.

I think what will happen is that at the end of ALTER EXTENSION,
we'll store the object's current ACL verbatim in pg_init_privs,
therefore including the effects of step 2.  This seems undesirable,
but I'm not sure how to get around it.


Anyway, this is starting to look like the sort of can of worms
best not opened post-beta1.  v17 has made some things better in this
area, and I don't think it's made anything worse; so maybe we should
declare victory for the moment and hope to address these additional
concerns later.  I've added an open item though.

regards, tom lane

Re: warn if GUC set to an invalid shared library

[Justin Pryzby]

On Fri, May 24, 2024 at 09:26:54AM -0400, Robert Haas wrote:
> On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby  wrote:

> But then I realized, reading another email, that Justin already knew
> that the behavior would change, or at least I'm 90% certain that he
> knows that.

You give me too much credit..

> On the behavior change itself, it seems to me that there's a big
> difference between shared_preload_libraries=bogus and work_mem=bogus.
..
> So if changing PGC_S_FILE to
> PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of
> allowing garbage values into postgresql.auto.conf that would currently
> get blocked, I think that's a bad plan and we shouldn't do it.

Right - this is something I'd failed to realize.  We can't change it in
the naive way because it allows bogus values, and not just missing
libraries.  Specifically, for GUCs with assign hooks conditional on
PGC_TEST.

We don't want to change the behavior to allow this to succeed -- it
would allow leaving the server in a state that it fails to start (rather
than helping to avoid doing so, as intended by this thread).

regression=# ALTER SYSTEM SET default_table_access_method=abc;
NOTICE:  table access method "abc" does not exist
ALTER SYSTEM

Maybe there should be a comment explaning why PGC_FILE is used, and
maybe there should be a TAP test for the behavior, but they're pretty
unrelated to this thread.  So, I've dropped the 001 patch.  

-- 
Justin
>From d29d0cfcf9d15dad7db1d0dd334e3e77cdad653f Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 1/3] warn when setting GUC to a nonextant library

Note that warnings shouldn't be issued during startup (only fatals):

$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL:  could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT:  while loading shared libraries for setting "shared_preload_libraries"
---
 src/backend/commands/variable.c   | 95 +++
 src/backend/utils/misc/guc_tables.c   |  6 +-
 src/include/utils/guc_hooks.h |  7 ++
 .../unsafe_tests/expected/rolenames.out   | 19 
 .../modules/unsafe_tests/sql/rolenames.sql| 13 +++
 src/test/regress/expected/rules.out   |  8 ++
 6 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 9345131711e..bab51c4572a 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
 #include "utils/timestamp.h"
 #include "utils/tzparser.h"
 #include "utils/varlena.h"
+#include 
 
 /*
  * DATESTYLE
@@ -1234,3 +1235,97 @@ check_ssl(bool *newval, void **extra, GucSource source)
 #endif
 	return true;
 }
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+		bool restricted)
+{
+	char	   *rawstring;
+	List	   *elemlist;
+	ListCell   *l;
+
+	/* nothing to do if empty */
+	if (newval == NULL || *newval[0] == '\0')
+		return true;
+
+	/*
+	 * issue warnings only during an interactive SET, from ALTER
+	 * ROLE/DATABASE/SYSTEM.
+	 */
+	if (source != PGC_S_TEST)
+		return true;
+
+	/* Need a modifiable copy of string */
+	rawstring = pstrdup(*newval);
+
+	/* Parse string into list of filename paths */
+	if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+	{
+		/* Should not happen ? */
+		return false;
+	}
+
+	foreach(l, elemlist)
+	{
+		/* Note that filename was already canonicalized */
+		char	   *filename = (char *) lfirst(l);
+		char	   *expanded = NULL;
+
+		/* If restricting, insert $libdir/plugins if not mentioned already */
+		if (restricted && first_dir_separator(filename) == NULL)
+		{
+			expanded = psprintf("$libdir/plugins/%s", filename);
+			filename = expanded;
+		}
+
+		/*
+		 * stat()/access() only check that the library exists, not that it has
+		 * the correct magic number or even a library.  But error messages
+		 * from dlopen() are not portable, so it'd be hard to report any
+		 * problem other than "does not exist".
+		 */
+		if (access(filename, R_OK) == 0)
+			continue;
+
+		if (source == PGC_S_FILE)
+			/* ALTER SYSTEM */
+			ereport(WARNING,
+	errcode_for_file_access(),
+	errmsg("could not access file \"%s\"", filename),
+	errdetail("The server will currently fail to start with this setting."),
+	errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+			"postgresql.auto.conf"));
+		else
+			/* ALTER ROLE/DATABASE */
+			ereport(WARNING,
+	errcode_for_file_access(),
+	errmsg("could not access file \"%s\"", filename),
+	errdetail("New sessions will currently fail to connect with the new setting."));
+	}
+
+	list_fr

Re: pg_parse_json() should not leak token copies on failure

[Jacob Champion]

On Tue, Apr 30, 2024 at 2:29 PM Jacob Champion
 wrote:
> Attached is a draft patch to illustrate what I mean, but it's
> incomplete: it only solves the problem for scalar values.

(Attached is a v2 of that patch; in solving a frontend leak I should
probably not introduce a backend segfault.)

--Jacob


v2-0001-WIP-fix-leak-of-scalar-value-on-lex-failure.patch
Description: Binary data

Re: commitfest.postgresql.org is no longer fit for purpose

[Tomas Vondra]

On 5/20/24 16:16, Robert Haas wrote:
> On Sun, May 19, 2024 at 3:18 PM Tom Lane  wrote:
>
>...
> 
>> * Another reason for things sitting a long time is that they're too
>> big to review without an unreasonable amount of effort.  We should
>> encourage authors to break large patches into smaller stepwise
>> refinements.  It may seem like that will result in taking forever
>> to reach the end goal, but dropping a huge patchset on the community
>> isn't going to give speedy results either.
> 
> Especially if it has a high rate of subtle defects, which is common.
> 

I think breaking large patches into reasonably small parts is a very
important thing. Not only is it really difficult (or perhaps even
practically impossible) to review patches over a certain size, because
you have to grok and review everything at once. But it's also not great
from the cost/benefit POV - the improvement may be very beneficial, but
if it's one huge lump of code that never gets committable as a whole,
there's no benefit in practice. Which makes it less likely I'll even
start looking at the patch very closely, because there's a risk it'd be
just a waste of time in the end.

So I think this is another important reason to advise people to split
patches into smaller parts - not only it makes it easier to review, it
makes it possible to review and commit the parts incrementally, getting
at least some benefits early.

>> * Before starting this thread, Robert did a lot of very valuable
>> review of some individual patches.  I think what prompted him to
>> start the thread was the realization that he'd only made a small
>> dent in the problem.  Maybe we could divide and conquer: get a
>> dozen-or-so senior contributors to split up the list of pending
>> patches and then look at each one with an eye to what needs to
>> happen to move it along (*not* to commit it right away, although
>> in some cases maybe that's the thing to do).  It'd be great if
>> that could happen just before each commitfest, but that's probably
>> not practical with the current patch volume.  What I'm thinking
>> for the moment is to try to make that happen once a year or so.
> 
> I like this idea.
> 

Me too. Do you think it'd happen throughout the whole year, or at some
particular moment?

We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
be more of an ad hoc thing to use the remaining time, with only a small
subset of people. But that seems pretty late in the dev cycle, I guess
we'd want it to happen early, perhaps during the first CF?

FWIW this reminds me the "CAN reports" tracking the current "conditions,
actions and needs" of a ticket. I do that for internal stuff, and I find
that quite helpful (as long as it's kept up to date).

>> Yeah, all this stuff ultimately gets done "for the good of the
>> project", which isn't the most reliable motivation perhaps.
>> I don't see a better one...
> 
> This is the really hard part.
> 

I think we have plenty of motivated people with good intentions. Some
are motivated by personal interest, some by trying to achieve stuff
related to their work/research/... I don't think the exact reasons
matter too much, and it's often a combination.

IMHO we should look at this from the other end - people are motivated to
get a patch reviewed & committed, and if we introduce a process that's
more likely to lead to that result, people will mostly adopt that.

And if we could make that process more convenient by improving the CF
app to support it, that'd be even better ... I'm mostly used to the
mailing list idea, but with the volume it's a constant struggle to keep
track of new patch versions that I wanted/promised to review, etc. The
CF app helps with that a little bit, because I can "become a reviewer"
but I still don't get notifications or anything like that :-(


regards

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

Re: Logging which local address was connected to in log_line_prefix

[Greg Sabino Mullane]

Peter, thank you for the feedback. Attached is a new patch with "address"
rather than "interface", plus a new default of "local" if there is no
address. I also removed the questionable comment, and updated the
commitfest title.

Cheers,
Greg
From bfa69fc2fffcb29dee0c6acfa4fc3749f987b272 Mon Sep 17 00:00:00 2001
From: Greg Sabino Mullane 
Date: Fri, 24 May 2024 11:25:48 -0400
Subject: [PATCH] Add local address to log_line_prefix

---
 doc/src/sgml/config.sgml  |  5 
 src/backend/utils/error/elog.c| 25 +++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 3 files changed, 31 insertions(+)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 698169afdb..d0b5e4d9ea 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7470,6 +7470,11 @@ local0.*/var/log/postgresql
  Remote host name or IP address
  yes
 
+
+ %L
+ Local address
+ yes
+
 
  %b
  Backend type
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d91a85cb2d..b1525d901c 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -67,6 +67,7 @@
 #endif
 
 #include "access/xact.h"
+#include "common/ip.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
@@ -79,6 +80,7 @@
 #include "storage/ipc.h"
 #include "storage/proc.h"
 #include "tcop/tcopprot.h"
+#include "utils/builtins.h"
 #include "utils/guc_hooks.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
@@ -3023,6 +3025,29 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
 	appendStringInfoSpaces(buf,
 		   padding > 0 ? padding : -padding);
 break;
+			case 'L':
+if (MyProcPort
+	&& (MyProcPort->laddr.addr.ss_family == AF_INET
+		||
+		MyProcPort->laddr.addr.ss_family == AF_INET6)
+	)
+{
+	Port *port = MyProcPort;
+	char local_host[NI_MAXHOST];
+
+	local_host[0] = '\0';
+
+	if (0 == pg_getnameinfo_all(&port->laddr.addr, port->laddr.salen,
+			   local_host, sizeof(local_host),
+			   NULL, 0,
+			   NI_NUMERICHOST | NI_NUMERICSERV)
+		)
+		appendStringInfo(buf, "%s", local_host);
+}
+else
+	appendStringInfo(buf, "[local]");
+
+break;
 			case 'r':
 if (MyProcPort && MyProcPort->remote_host)
 {
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 83d5df8e46..85a9c59116 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -587,6 +587,7 @@
 	#   %d = database name
 	#   %r = remote host and port
 	#   %h = remote host
+#   %L = local address
 	#   %b = backend type
 	#   %p = process ID
 	#   %P = process ID of parallel group leader
-- 
2.30.2

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Daniel Gustafsson]

> On 24 May 2024, at 16:20, Tom Lane  wrote:

> I've tentatively concluded that I shouldn't have modeled
> SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL,
> in particular the decision that we don't need such an entry if
> there's also SHARED_DEPENDENCY_OWNER.  

+1, in light of this report I think we need to go back on that.

> I can see two routes to a solution:
> 
> 1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the
> role is the object's owner or not.  Then, clearing out the
> pg_shdepend entry cues us to go delete the pg_init_privs entry.
> 
> 2. Just always search pg_init_privs for relevant entries
> when dropping an object.
> 
> I don't especially like #2 on performance grounds, but it has
> a lot fewer moving parts than #1.

#1 is more elegant, but admittedly also more complicated.  An unscientific
guess is that a majority of objects dropped won't have init privs, making the
extra scan in #2 quite possibly more than academic.  #2 could however be
backported and solve the issue in existing clusters.

> Another point: shdepReassignOwned explicitly does not touch grants
> or default ACLs.  It feels like the same should be true of
> pg_init_privs entries,

Agreed, I can't see why pg_init_privs should be treated differently.

> Another thing I'm wondering about right now is privileges on global
> objects (roles, databases, tablespaces).  The fine manual says
> "Although an extension script is not prohibited from creating such
> objects, if it does so they will not be tracked as part of the
> extension".  Presumably, that also means that no pg_init_privs
> entries are made; but do we do that correctly?

I'm away from a tree to check, but that does warrant investigation.  If we
don't have a test for it already then it might be worth constructing something
to catch that.

--
Daniel Gustafsson

Re: Convert node test compile-time settings into run-time parameters

[Tom Lane]

Peter Eisentraut  writes:
> Ok, I have an improved plan.  I'm wrapping all the code related to this 
> in #ifdef DEBUG_NODE_TESTS_ENABLED.  This in turn is defined in 
> assert-enabled builds, or if you define it explicitly, or if you define 
> one of the legacy individual symbols.  That way you get the run-time 
> settings in a normal development build, but there is no new run-time 
> overhead.  This is the same scheme that we use for debug_discard_caches.

+1; this addresses my concern about not adding effectively-dead code
to production builds.  Your point upthread about debug_print_plan and
other legacy debug switches was not without merit; should we also fold
those into this plan?  (In that case we'd need a symbol named more
generically than DEBUG_NODE_TESTS_ENABLED.)

> (An argument could be made to enable this code if and only if assertions 
> are enabled, since these tests are themselves kind of assertions.  But I 
> think having a separate symbol documents the purpose of the various code 
> sections better.)

Agreed.

>> Maybe not three settings, but a single setting, with multiple values, like
>> debug_io_direct?

> Yeah, good idea.  Let's get some more feedback on this before I code up 
> a complicated list parser.

Kinda doubt it's worth the trouble, either to code the GUC support or
to use it.  I don't object to having the booleans in a debug build,
I was just concerned about whether they should exist in production.

regards, tom lane

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

Daniel Gustafsson  writes:
> I had a look, but I didn't beat you to a fix since it's not immediately clear
> to me how this should work for REASSING OWNED (DROP OWNED seems a simpler
> case).  Should REASSIGN OWNED alter the rows in pg_shdepend matching init 
> privs
> from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can 
> be
> mopped up with a later DROP OWNED?  Trying this in a POC patch it fails with
> RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test
> seems to make it work but is it the right approach?

I've tentatively concluded that I shouldn't have modeled
SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL,
in particular the decision that we don't need such an entry if
there's also SHARED_DEPENDENCY_OWNER.  I think one reason we
can get away with omitting a SHARED_DEPENDENCY_ACL entry for the
owner is that the object's normal ACL is part of its primary
catalog row, so it goes away automatically if the object is
dropped.  But obviously that's not true for a pg_init_privs
entry.  I can see two routes to a solution:

1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the
role is the object's owner or not.  Then, clearing out the
pg_shdepend entry cues us to go delete the pg_init_privs entry.

2. Just always search pg_init_privs for relevant entries
when dropping an object.

I don't especially like #2 on performance grounds, but it has
a lot fewer moving parts than #1.  In particular, there's some
handwaving in changeDependencyOnOwner() about why we should
drop SHARED_DEPENDENCY_ACL when changing owner, and I've not
wrapped my head around how those concerns map to INITACL
if we treat it in this different way.

Another point: shdepReassignOwned explicitly does not touch grants
or default ACLs.  It feels like the same should be true of
pg_init_privs entries, or at least if not, why not?  In that case
there's nothing to be done in shdepReassignOwned (although maybe its
comments should be adjusted to mention this explicitly).  The bug is
just that DROP OWNED isn't getting rid of the entries because there's
no INITACL entry to cue it to do so.

Another thing I'm wondering about right now is privileges on global
objects (roles, databases, tablespaces).  The fine manual says
"Although an extension script is not prohibited from creating such
objects, if it does so they will not be tracked as part of the
extension".  Presumably, that also means that no pg_init_privs
entries are made; but do we do that correctly?

Anyway, -ENOCAFFEINE for the moment.  I'll look more later.

regards, tom lane

Re: Upgrade Debian CI images to Bookworm

[Peter Eisentraut]

On 13.05.24 12:57, Nazir Bilal Yavuz wrote:

Bookworm versions of the Debian CI images are available now [0]. The
patches to use these images are attached.

'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_16+.patch' patch can
be applied to both upstream and REL_16 and all of the tasks finish
successfully.

'v1-0001-Upgrade-Debian-CI-images-to-Bookworm_REL_15.patch' patch can
be applied to REL_15 but it gives a compiler warning. The fix for this
warning is proposed here [1]. After the fix is applied, all of the
tasks finish successfully.

Any kind of feedback would be appreciated.


These updates are very welcome and look straightforward enough.

I'm not sure what the backpatching expectations of this kind of thing 
is.  The history of this CI setup is relatively short, so this hasn't 
been stressed too much.  I see that we once backpatched the macOS 
update, but that might have been all.


If we start backpatching this kind of thing, then this will grow as a 
job over time.  We'll have 5 or 6 branches to keep up to date, with 
several operating systems.  And once in a while we'll have to make 
additional changes like this warning fix you mention here.  I'm not sure 
how much we want to take this on.  Is there ongoing value in the CI 
setup in backbranches?


With these patches, we could do either of the following:

1) We update only master and only after it branches for PG18.  (The 
update is a "new feature".)


2) We update only master but do it now.  (This gives us the most amount 
of buffer time before the next release.)


3) We update master and PG16 now.  We ignore PG15.

4) We update master and PG16 now.  We update PG15 whenever that warning 
is fixed.


5) We update master, PG16, and PG15, but we hold all of them until the 
warning in PG15 is fixed.


6) We update all of them now and let the warning in PG15 be fixed 
independently.

Re: Question: Why Are File Descriptors Not Closed and Accounted for PostgreSQL Backends?

[Srinath Reddy Sadipiralla]

Thanks for the reply,yeah i know about FreeWaitEventSet() but that is being 
used in few places but not for handling backends.

i got it that FDs like FeBeWaitSet->epoll_fd will be free'd when connection is 
terminated but as i mentioned wouldn't it be an issue if the connection is long 
living lets take idle which can be running queries for long time,what if we 
have multiple connections like this running queries using multiple system FDs 
and reach the limit,cause they are using FDs ,so they may not be free'd.







 On Fri, 24 May 2024 19:15:54 +0530 Heikki Linnakangas  
wrote ---



On 24/05/2024 15:17, Srinath Reddy Sadipiralla wrote: 
> Hi PostgreSQL Community, 
> when a backend process starts, pq_init is called where it opens a FD during 
> CreateWaitEventSet() 
> 
> 
> if (!AcquireExternalFD()) 
> { 
> /* treat this as though epoll_create1 itself returned EMFILE */ 
> elog(ERROR, "epoll_create1 failed: %m"); 
> } 
> set->epoll_fd = epoll_create1(EPOLL_CLOEXEC); 
> 
> 
> but we didn't closed or called ReleaseExternalFD() for accounting 
 
Yes we do, see FreeWaitEventSet(). 
 
The WaitEventSet created fro pq_init() is never explicitly free'd 
though, because it's created in the per-connection backend process. When 
the connection is terminated, the backend process exits, cleaning up any 
resources including the WaitEventSet. 
 
-- 
Heikki Linnakangas 
Neon (https://neon.tech)

Re: PG catalog

[Tom Lane]

"Karki, Sanjay"  writes:
> I need to grant select on privilege in pg_catalog to user so I can connect 
> via Toad Data point ,

Why do you think you need to do that?  Most catalogs have public
select privilege already, and for the ones that don't, there are
very good reasons why not.  I don't know what "Toad Data point"
is, but if it thinks it needs more privilege than is normally
granted, you should be asking very pointed questions about why
and why that shouldn't be considered a security breach.

(Usually we get complaints that the default permissions on the
catalogs are too loose, not too tight.)

regards, tom lane

Re: First draft of PG 17 release notes

[Robert Haas]

On Thu, May 23, 2024 at 11:04 PM Bruce Momjian  wrote:
> For a case where O(N^2) become O(N), we might not even know the
> performance change since it is a micro-optimization.  That is why I
> suggested we call it "Internal Performance".

I don't get this at all. If we can't tell the difference between
O(N^2) and O(N), then N was small enough that there wasn't any real
need to optimize in the first place. I think we should be assuming
that if somebody took the trouble to write a patch, the difference did
matter. Hence the change would be user-visible, and should be
documented.

"Internal Performance" doesn't make a lot of sense to me as a section
heading. What does "Internal" mean here as opposed to "General"? I
suspect you mean to imply that the user won't be able to tell the
difference, but I doubt that very much. We make performance
improvements because they are user-visible. If a performance
improvement is so miniscule that nobody would ever notice the
difference, well then I don't think it needs to be release-noted at
all, and we might have a few changes like that where people were
mostly aiming for code cleanliness. But in general, what people do is
look for something that's slow (for them) and try to make it faster.
So the presumption should be that a performance feature has a
meaningful impact on users, and then in rare cases we may decide
otherwise.

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

Re: Question: Why Are File Descriptors Not Closed and Accounted for PostgreSQL Backends?

[Heikki Linnakangas]

On 24/05/2024 15:17, Srinath Reddy Sadipiralla wrote:

Hi PostgreSQL Community,
when a backend process starts, pq_init is called where it opens a FD during 
CreateWaitEventSet()


if (!AcquireExternalFD())
{
/* treat this as though epoll_create1 itself returned EMFILE */
elog(ERROR, "epoll_create1 failed: %m");
}
set->epoll_fd = epoll_create1(EPOLL_CLOEXEC);


but we didn't closed or called ReleaseExternalFD() for accounting


Yes we do, see FreeWaitEventSet().

The WaitEventSet created fro pq_init() is never explicitly free'd 
though, because it's created in the per-connection backend process. When 
the connection is terminated, the backend process exits, cleaning up any 
resources including the WaitEventSet.


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

[Robert Haas]

On Thu, May 23, 2024 at 4:00 PM Tom Lane  wrote:
> I don't recall exactly what I thought earlier, but now I think we'd
> be better off with separate infrastructure.  guc.c is unduly complex
> already.  Perhaps there are bits of it that could be factored out
> and shared, but I bet not a lot.

OK. That seems fine to me, but I bet Jelte is going to disagree.

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

Re: warn if GUC set to an invalid shared library

[Robert Haas]

On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby  wrote:
> I'm still hoping.

Hi,

I got asked to take a look at this thread.

First, I want to explain why I think this thread hasn't gotten as much
feedback as Justin was hoping. It is always possible for any thread to
have that problem just because people are busy or not interested.
However, I think in this case an aggravating factor is that the
discussion is very "high context"; it's hard to understand what the
open problems are without reading a lot of emails and understanding
how they all relate to each other. One of the key questions is whether
we should replace PGC_S_FILE with PGC_S_TEST in
AlterSystemSetConfigFile. I originally thought, based on reading one
of the emails, that the question was whether we should do that out of
some sense of intellectual purity, and my answer was "probably not,
because that would change the behavior in a way that doesn't seem
good." But then I realized, reading another email, that Justin already
knew that the behavior would change, or at least I'm 90% certain that
he knows that. So now I think the question is whether we want that
behavior change, but he only provided one example of how the behavior
changes, and it's not clear how many other scenarios are affected or
in what way, so it's still a bit hard to answer. Plus, it took me 10
minutes to figure out what the question was. I think that if the
question had been phrased in a way that was easily understandable to
any experienced PostgreSQL user, it's a lot more likely that one or
more people would have had an opinion on whether it was good or bad.
As it is, I think most people probably didn't understand the question,
and the people who did understand the question may not have wanted to
spend the time to do the research that they would have needed to do to
come up with an intelligent answer. I'm not saying any of this to
criticize Justin or to say that he did anything wrong, but I think we
have lots of examples of stuff like this on the mailing list, where
people are sad because they didn't get an answer, but don't always
realize that there might be things they could do to improve their
chances.

On the behavior change itself, it seems to me that there's a big
difference between shared_preload_libraries=bogus and work_mem=bogus.
The former is valid or invalid according to whether bogus.so exists in
an appropriate directory on the local machine, but the latter is
categorically invalid. I'm not sure to what degree we have the
infrastructure to distinguish those cases, but to the extent that we
do, handling them differently is completely defensible. It's
reasonable to allow the first one on the theory that the
presently-invalid configuration may at a later time become valid, but
that's not reasonable in the second case. So if changing PGC_S_FILE to
PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of
allowing garbage values into postgresql.auto.conf that would currently
get blocked, I think that's a bad plan and we shouldn't do it. But
it's quite possible I'm not fully understanding the situation.

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

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

>
>
>
>
>>
>> As far as I can see now, it's a major design flaw that could keep the
>> patch from being accepted. Fortunately there are few good proposals how
>> to address this, folks are genuinely trying to help. What do you think
>> about trying some of them out, as an alternative approach, to compare
>> functionality and user experience?
>>
>
> It is a design flaw of SQL. The issue we talk about is the generic
> property of SQL, and then you cannot fix it.
>
> I thought about possibility to introduce dedicated function
>
> svalue(regvariable) returns any - with planner support
>
> and possibility to force usage of this function. Another possibility is
> using some simple dedicated operator (syntax) for force using of variables
> so theoretically this can looks like:
>
> set strict_usage_of_session_variables to on;
> SELECT * FROM tab WHERE a = svalue('myvar.var');
> or
>
> SELECT * FROM tab WHERE a = @ myvar.var;
>
> This can be really safe. Personally It is not my cup of tea, but I can
> live it (and this mode can be default).
>
> Theoretically we can limit usage of variables just for PL/pgSQL. It can
> reduce risks too, but it breaks usage variables for parametrization of DO
> blocks (what is my primary motivation), but it can be good enough to
> support migration from PL/SQL.
>

another possibility can be disable / enable usage of session variables on
session level

like set enable_session_variable to on/off

so when the application doesn't use session variables, and then session
variables can be disabled, but the user can enable it just for self for
self session. Then the risk of unwanted usage of session variables can be
zero. This is similar to discussion about login triggers. This mechanism
can be used for using session variables only in PL too.




>
>
>>
>> In the meantime I'm afraid I have to withdraw "Ready for committer"
>> status, sorry. I've clearly underestimated the importance of variables
>> shadowing, thanks Alvaro and Peter for pointing out some dangerous
>> cases. I still believe though that the majority of the patch is in a
>> good shape and the question about variables shadowing is the only thing
>> that keeps it from moving forward.
>>
>
> I understand.
>
> I'll try to recapitulate my objections against proposed designs
>
> a) using syntax like MS - DECLARE command and '@@' prefix - it is dynamic,
> so there is not possibility of static check. It is not joined with schema,
> so there are possible collisions between variables and and the end the
> variables are named like @@mypackage_myvar - so some custom naming
> convention is necessary too. There is not possibility to set access rights.
>
> b) using variables like MySQL - first usage define it, and access by '@'
> prefix. It is simple, but without possibility of static check. There is not
> possibility to set access rights.
>
> c) using variables with necessity to define it in FROM clause. It is safe,
> but it can be less readable, when you use more variables, and it is not too
> readable, and user friendly, because you need to write FROM. And can be
> messy, because you usually will use variables in queries, and it is
> introduce not relations into FROM clause. But I can imagine this mode as
> alternative syntax, but it is very unfriendly and not intuitive (I think).
> More probably it doesn't fast execution in simple expression execution mode.
>
> d) my proposal - there is possibility of collisions, but consistent with
> naming of database objects, allows set of access rights, allows static
> analyze, consistent with PL/pgSQL and similar to PL/pgSQL.
>
> There is not any other possibility. Any time this is war between be user
> friendly, be readable, be correctly - but there is not perfect solution,
> because just SQL is not perfect. Almost all mentioned objections against
> proposed variables are valid just for tables and columns.
>
> Regards
>
> Pavel
>
>

Re: Wrong results with grouping sets

[Richard Guo]

On the basis of the parser infrastructure fixup, 0002 patch adds the
nullingrel bit that references the grouping RTE to the grouping
expressions.

However, it seems to me that we have to manually remove this nullingrel
bit from expressions in various cases where these expressions are
logically below the grouping step, such as when we generate groupClause
pathkeys for grouping sets, or when we generate PathTarget for initial
input to grouping nodes.

Furthermore, in set_upper_references, the targetlist and quals of an Agg
node should have nullingrels that include the effects of the grouping
step, ie they will have nullingrels equal to the input Vars/PHVs'
nullingrels plus the nullingrel bit that references the grouping RTE.
In order to perform exact nullingrels matches, I think we also need to
manually remove this nullingrel bit.

Thanks
Richard


v6-0001-Introduce-a-RTE-for-the-grouping-step.patch
Description: Binary data


v6-0002-Mark-expressions-nullable-by-grouping-sets.patch
Description: Binary data

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

Hi

pá 24. 5. 2024 v 13:32 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
napsal:

> > On Wed, May 22, 2024 at 08:44:28PM +0200, Pavel Stehule wrote:
> > st 22. 5. 2024 v 19:25 odesílatel Tom Lane  napsal:
> >
> > > Peter Eisentraut  writes:
> > > > On 18.05.24 13:29, Alvaro Herrera wrote:
> > > >> I want to note that when we discussed this patch series at the dev
> > > >> meeting in FOSDEM, a sort-of conclusion was reached that we didn't
> want
> > > >> schema variables at all because of the fact that creating a variable
> > > >> would potentially change the meaning of queries by shadowing table
> > > >> columns.  But this turns out to be incorrect: it's_variables_  that
> are
> > > >> shadowed by table columns, not the other way around.
> > >
> > > > But that's still bad, because seemingly unrelated schema changes can
> > > > make variables appear and disappear.  For example, if you have
> > > >   SELECT a, b FROM table1
> > > > and then you drop column b, maybe the above query continues to work
> > > > because there is also a variable b.
> > >
> > > Yeah, that seems pretty dangerous.  Could we make it safe enough
> > > by requiring some qualification on variable names?  That is, if
> > > you mean b to be a variable, then you must write something like
> > >
> > > SELECT a, pg_variables.b FROM table1
> > >
> > > This is still ambiguous if you use "pg_variables" as a table alias in
> > > the query, but the alias would win so the query still means what it
> > > meant before.  Also, table aliases (as opposed to actual table names)
> > > don't change readily, so I don't think there's much risk of the query
> > > suddenly meaning something different than it did yesterday.
> > >
> >
> > With active shadowing variable warning for described example you will
> get a
> > warning before dropping.
>
> I assume you're talking about a warning, which one will get querying the
> table with shadowed columns. If no such query has happened yet and the
> column was dropped, there will be no warning.
>

sure - the possible identifier collision cannot be solved in SQL perfectly.
It is the same with tables.
When I add badly named column to table, I'll get an error "ambiguous
columns" just when I'll execute
query. The system catalog just cannot protect against collisions - it is
true for columns, variables, tables.
Little bit protected are views, that are stored in parsed format, but any
other object can be broken when
somebody choose bad names in catalog or queries. There is not any
protection.


>
> Aside that, I'm afraid dropping a warning in log does not have
> sufficient visibility to warn about the issue, since one needs to read
> those logs first. I guess what folks are looking for is more constraints
> out of the box, preventing any ambiguity.
>

We can increase (optionality) the level of this message to error. It is not
perfect, but it can work well.

I think so there is not higher risk with variables than current risk with
just tables.

a) the possibility to create variables is limited by rights on schema. So
nobody can create variables without necessary rights (invisibly)

b) if user has own schema with CREATE right, then it can create variables
just for self, and with default setting, just visible for self,
and just accessible for self. When other users try to use these variables,
then the query fails due to missing access rights (usually).
Common user cannot to create variables in application schema and cannot to
set search_path for applications.

c) the changes of schema are usually tested on some testing stages before
are applied on production. So when there
will be possible collision or some other defect, probably it will be
detected there. Untested changes of catalog on production is not too common
today.

d) any risk that can be related for variables, is related just to renaming
column or table.



>
> > Session variables are joined with schema (in my proposal). Do anybody can
> > do just
> >
> > CREATE SCHEMA svars; -- or what (s)he likes
> > CREATE VARIABLE svars.b AS int;
> >
> > SELECT a, b FROM table1
> >
> > and if somebody can be really safe, the can write
> >
> > SELECT t.a, t.b FROM table1 t
> >
> > or
> >
> > SELECT t.a, svars.b FROM table1 t
> >
> > It can be customized in the way anybody prefers - just creating dedicated
> > schemas and setting search_path. Using its own schema for session
> variables
> > without enhancing search_path for this schema forces the necessity to set
> > only qualified names for session variables.
> >
> > Sure the naming of schemas, aliases can be unhappy wrong, and there can
> be
> > the problem. But this can be a problem today too.
>
> If I understand you correctly, you're saying that there are "best
> practices" how to deal with session variables to avoid any potential
> issues. But I think it's more user-friendly to have something that will
> not allow shooting yourself in the foot right out of the box. You're
> right, similar things could probably happe

Re: 回复: An implementation of multi-key sort

[Yao Wang]

When all leading keys are different, mksort will finish the entire sort at the
first sort key and never touch other keys. For the case, mksort falls back to
kind of qsort actually.

I created another data set with distinct values in all sort keys:

create table t2 (c1 int, c2 int, c3 int, c4 int, c5 int, c6 varchar(100));
insert into t2 values (generate_series(1,49), 0, 0, 0, 0, '');
update t2 set c2 = 90 - c1, c3 = 91 - c1, c4 = 92 - c1, c5
= 93 - c1;
update t2 set c6 = 'aaabbb'
  || (94 - c1)::text;
explain analyze select c1 from t2 order by c6, c5, c4, c3, c2, c1;

Results:

MKsort:
12374.427 ms
12528.068 ms
12554.718 ms

qsort:
12251.422 ms
12279.938 ms
12280.254 ms

MKsort is a bit slower than qsort, which can be explained by extra
checks of MKsort.

Yao Wang

On Fri, May 24, 2024 at 8:36 PM Wang Yao  wrote:
>
>
>
> 获取Outlook for Android
> 
> From: Heikki Linnakangas 
> Sent: Thursday, May 23, 2024 8:47:29 PM
> To: Wang Yao ; PostgreSQL Hackers 
> 
> Cc: inte...@outlook.com 
> Subject: Re: 回复: An implementation of multi-key sort
>
> On 23/05/2024 15:39, Wang Yao wrote:
> > No obvious perf regression is expected because PG will follow original
> > qsort code path when mksort is disabled. For the case, the only extra
> > cost is the check in tuplesort_sort_memtuples() to enter mksort code path.
>
> And what about the case the mksort is enabled, but it's not effective
> because all leading keys are different?
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Question: Why Are File Descriptors Not Closed and Accounted for PostgreSQL Backends?

[Srinath Reddy Sadipiralla]

Hi PostgreSQL Community, 
when a backend process starts, pq_init is called where it opens a FD during 
CreateWaitEventSet()


if (!AcquireExternalFD())
{
/* treat this as though epoll_create1 itself returned EMFILE */
elog(ERROR, "epoll_create1 failed: %m");
}
set->epoll_fd = epoll_create1(EPOLL_CLOEXEC);


but we didn't closed or called ReleaseExternalFD() for accounting,lets say if 
we have multiple clients connected and are actively running queries, won't the 
max number of open FDs (ulimit -n) limit of the system gets reached and cause 
"Too many open files issue"?

Regards
Srinath Reddy

Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)

[Ranier Vilela]

Em sex., 24 de mai. de 2024 às 08:48, Ashutosh Bapat <
ashutosh.bapat@gmail.com> escreveu:

>
>
> On Fri, May 24, 2024 at 12:16 PM Michael Paquier 
> wrote:
>
>> On Fri, May 24, 2024 at 11:58:51AM +0530, Ashutosh Bapat wrote:
>> > If we are looking for avoiding a segfault and get a message which helps
>> > debugging, using get_attname and get_attnum might be better options.
>> > get_attname throws an error. get_attnum doesn't throw an error and
>> returns
>> > InvalidAttnum which won't return any valid identity sequence, and thus
>> > return a NIL sequence list which is handled in that function already.
>> Using
>> > these two functions will avoid the clutter as well as segfault. If
>> that's
>> > acceptable, I will provide a patch.
>>
>> Yeah, you could do that with these two routines as well.  The result
>> would be the same in terms of runtime validity checks.
>>
>
> PFA patch using those two routines.
>
The function *get_attname* palloc the result name (pstrdup).
Isn't it necessary to free the memory here (pfree)?

best regards,
Ranier Vilela

Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)

[Ashutosh Bapat]

On Fri, May 24, 2024 at 12:16 PM Michael Paquier 
wrote:

> On Fri, May 24, 2024 at 11:58:51AM +0530, Ashutosh Bapat wrote:
> > If we are looking for avoiding a segfault and get a message which helps
> > debugging, using get_attname and get_attnum might be better options.
> > get_attname throws an error. get_attnum doesn't throw an error and
> returns
> > InvalidAttnum which won't return any valid identity sequence, and thus
> > return a NIL sequence list which is handled in that function already.
> Using
> > these two functions will avoid the clutter as well as segfault. If that's
> > acceptable, I will provide a patch.
>
> Yeah, you could do that with these two routines as well.  The result
> would be the same in terms of runtime validity checks.
>

PFA patch using those two routines.

-- 
Best Wishes,
Ashutosh Bapat
From 2390604e8b77a3e67693991cfcac596afc7b8572 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Fri, 24 May 2024 16:50:00 +0530
Subject: [PATCH] Fix potential NULL pointer dereference in
 getIdentitySequence()

The function invokes SearchSysCacheAttNum() and SearchSysCacheAttName(). Both
of them may return NULL if the attribute number or attribute name does not
exist. The function does not check for returned NULL value since it never
passes non-existing attribute values. Though the risk of segmentation fault
because of dereferencing NULL value does not exist today, it may arise in
future. Fix it by using wrappers get_attnum() and get_attname() respectively.
The second function raises an error when non-existing attribute name is passed
to it. The first returns InvalidAttNum which will make
getOwnedSequences_internal() return NIL resulting in an error.

Author: Ashutosh Bapat
Reported by: Ranier Vilela
Discussion: https://www.postgresql.org/message-id/CAEudQAqh_RZqoFcYKso5d9VhF-Vd64_ZodfQ_2zSusszkEmyRg%40mail.gmail.com
---
 src/backend/catalog/pg_depend.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 5366f7820c..6422c27591 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -945,7 +945,7 @@ getOwnedSequences(Oid relid)
 Oid
 getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
 {
-	Oid			relid;
+	Oid			relid = RelationGetRelid(rel);
 	List	   *seqlist;
 
 	/*
@@ -954,22 +954,13 @@ getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
 	 */
 	if (RelationGetForm(rel)->relispartition)
 	{
-		List	   *ancestors =
-			get_partition_ancestors(RelationGetRelid(rel));
-		HeapTuple	ctup = SearchSysCacheAttNum(RelationGetRelid(rel), attnum);
-		const char *attname = NameStr(((Form_pg_attribute) GETSTRUCT(ctup))->attname);
-		HeapTuple	ptup;
+		List	   *ancestors = get_partition_ancestors(relid);
+		const char *attname = get_attname(relid, attnum, false);
 
 		relid = llast_oid(ancestors);
-		ptup = SearchSysCacheAttName(relid, attname);
-		attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
-
-		ReleaseSysCache(ctup);
-		ReleaseSysCache(ptup);
+		attnum = get_attnum(relid, attname);
 		list_free(ancestors);
 	}
-	else
-		relid = RelationGetRelid(rel);
 
 	seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL);
 	if (list_length(seqlist) > 1)
-- 
2.34.1

Re: Schema variables - new implementation for Postgres 15

[Dmitry Dolgov]

> On Wed, May 22, 2024 at 08:44:28PM +0200, Pavel Stehule wrote:
> st 22. 5. 2024 v 19:25 odesílatel Tom Lane  napsal:
>
> > Peter Eisentraut  writes:
> > > On 18.05.24 13:29, Alvaro Herrera wrote:
> > >> I want to note that when we discussed this patch series at the dev
> > >> meeting in FOSDEM, a sort-of conclusion was reached that we didn't want
> > >> schema variables at all because of the fact that creating a variable
> > >> would potentially change the meaning of queries by shadowing table
> > >> columns.  But this turns out to be incorrect: it's_variables_  that are
> > >> shadowed by table columns, not the other way around.
> >
> > > But that's still bad, because seemingly unrelated schema changes can
> > > make variables appear and disappear.  For example, if you have
> > >   SELECT a, b FROM table1
> > > and then you drop column b, maybe the above query continues to work
> > > because there is also a variable b.
> >
> > Yeah, that seems pretty dangerous.  Could we make it safe enough
> > by requiring some qualification on variable names?  That is, if
> > you mean b to be a variable, then you must write something like
> >
> > SELECT a, pg_variables.b FROM table1
> >
> > This is still ambiguous if you use "pg_variables" as a table alias in
> > the query, but the alias would win so the query still means what it
> > meant before.  Also, table aliases (as opposed to actual table names)
> > don't change readily, so I don't think there's much risk of the query
> > suddenly meaning something different than it did yesterday.
> >
>
> With active shadowing variable warning for described example you will get a
> warning before dropping.

I assume you're talking about a warning, which one will get querying the
table with shadowed columns. If no such query has happened yet and the
column was dropped, there will be no warning.

Aside that, I'm afraid dropping a warning in log does not have
sufficient visibility to warn about the issue, since one needs to read
those logs first. I guess what folks are looking for is more constraints
out of the box, preventing any ambiguity.

> Session variables are joined with schema (in my proposal). Do anybody can
> do just
>
> CREATE SCHEMA svars; -- or what (s)he likes
> CREATE VARIABLE svars.b AS int;
>
> SELECT a, b FROM table1
>
> and if somebody can be really safe, the can write
>
> SELECT t.a, t.b FROM table1 t
>
> or
>
> SELECT t.a, svars.b FROM table1 t
>
> It can be customized in the way anybody prefers - just creating dedicated
> schemas and setting search_path. Using its own schema for session variables
> without enhancing search_path for this schema forces the necessity to set
> only qualified names for session variables.
>
> Sure the naming of schemas, aliases can be unhappy wrong, and there can be
> the problem. But this can be a problem today too.

If I understand you correctly, you're saying that there are "best
practices" how to deal with session variables to avoid any potential
issues. But I think it's more user-friendly to have something that will
not allow shooting yourself in the foot right out of the box. You're
right, similar things could probably happen with the already existing
functionality, but it doesn't give us rights to add more to it.
Especially if it's going to be about a brand-new feature.

As far as I can see now, it's a major design flaw that could keep the
patch from being accepted. Fortunately there are few good proposals how
to address this, folks are genuinely trying to help. What do you think
about trying some of them out, as an alternative approach, to compare
functionality and user experience?

In the meantime I'm afraid I have to withdraw "Ready for committer"
status, sorry. I've clearly underestimated the importance of variables
shadowing, thanks Alvaro and Peter for pointing out some dangerous
cases. I still believe though that the majority of the patch is in a
good shape and the question about variables shadowing is the only thing
that keeps it from moving forward.

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Daniel Gustafsson]

> On 24 May 2024, at 01:01, Tom Lane  wrote:
> 
> Hannu Krosing  writes:
>> While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
>> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
>> is still there:
> 
> Ugh, how embarrassing.  I'll take a look tomorrow, if no one
> beats me to it.

I had a look, but I didn't beat you to a fix since it's not immediately clear
to me how this should work for REASSING OWNED (DROP OWNED seems a simpler
case).  Should REASSIGN OWNED alter the rows in pg_shdepend matching init privs
from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can be
mopped up with a later DROP OWNED?  Trying this in a POC patch it fails with
RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test
seems to make it work but is it the right approach?

--
Daniel Gustafsson

Re: Improving tracking/processing of buildfarm test failures

[Amit Kapila]

On Thu, May 23, 2024 at 4:30 PM Alexander Lakhin  wrote:
>
> I'd like to discuss ways to improve the buildfarm experience for anyone who
> are interested in using information which buildfarm gives to us.
>
> Unless I'm missing something, as of now there are no means to determine
> whether some concrete failure is known/investigated or fixed, how
> frequently it occurs and so on... From my experience, it's not that
> unbelievable that some failure occurred two years ago and lost in time was
> an indication of e. g. a race condition still existing in the code/tests
> and thus worth fixing. But without classifying/marking failures it's hard
> to find such or other interesting failure among many others...
>
> The first way to improve things I can imagine is to add two fields to the
> buildfarm database: a link to the failure discussion (set when the failure
> is investigated/reproduced and reported in -bugs or -hackers) and a commit
> id/link (set when the failure is fixed). I understand that it requires
> modifying the buildfarm code, and adding some UI to update these fields,
> but it allows to add filters to see only unknown/non-investigated failures
> in the buildfarm web interface later.
>
> The second way is to create a wiki page, similar to "PostgreSQL 17 Open
> Items", say, "Known buildfarm test failures" and fill it like below:
> 
> 
> ...
> Useful info from the failure logs for reference
> ...
> 
> ---
> This way is less invasive, but it would work well only if most of
> interested people know of it/use it.
> (I could start with the second approach, if you don't mind, and we'll see
> how it works.)
>

I feel it is a good idea to do something about this. It makes sense to
start with something simple and see how it works. I think this can
also help us whether we need to chase a particular BF failure
immediately after committing.

-- 
With Regards,
Amit Kapila.

Re: PG catalog

[David G. Johnston]

On Thursday, May 23, 2024, Karki, Sanjay  wrote:
>
> I need to grant select on privilege in pg_catalog to user so I can connect
> via Toad Data point ,
>
> Users can already select from the tables in pg_catalog, grant able
privileges not required or allowed.  Of course, some specific data is
restricted from non-superusers.

David J.

Re: Convert node test compile-time settings into run-time parameters

[Peter Eisentraut]

On 21.05.24 20:48, Andres Freund wrote:

Where I'd be more concerned about peformance is the added branch in
READ_LOCATION_FIELD. There are a lot of calls to that, addding runtime
branches to each, with external function calls inside, is somewhat likely to
be measurable.


Ok, I have an improved plan.  I'm wrapping all the code related to this 
in #ifdef DEBUG_NODE_TESTS_ENABLED.  This in turn is defined in 
assert-enabled builds, or if you define it explicitly, or if you define 
one of the legacy individual symbols.  That way you get the run-time 
settings in a normal development build, but there is no new run-time 
overhead.  This is the same scheme that we use for debug_discard_caches.


(An argument could be made to enable this code if and only if assertions 
are enabled, since these tests are themselves kind of assertions.  But I 
think having a separate symbol documents the purpose of the various code 
sections better.)



Another thought:  Do we really need three separate settings?


Maybe not three settings, but a single setting, with multiple values, like
debug_io_direct?


Yeah, good idea.  Let's get some more feedback on this before I code up 
a complicated list parser.


Another approach might be levels.  My testing showed that the overhead 
of the copy_parse_plan_trees and raw_expression_coverage_tests flags is 
hardly noticeable, but write_read_parse_plan_trees has some noticeable 
impact.  So you could do 0=off, 1=only the cheap ones, 2=all tests.


In fact, if we could make "only the cheap ones" the default for 
assert-enabled builds, then most people won't even need to worry about 
this setting: The only way to mess up the write_read_parse_plan_trees is 
if you write custom node support, which is rare.  But the raw expression 
coverage still needs to be maintained by hand, so it's more often 
valuable to have it checked automatically.
From 80f35c90e3414dabd879e8832ce1b89c685e5de5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 24 May 2024 11:42:02 +0200
Subject: [PATCH v2] Convert node test compile-time settings into run-time
 parameters

This converts

COPY_PARSE_PLAN_TREES
WRITE_READ_PARSE_PLAN_TREES
RAW_EXPRESSION_COVERAGE_TEST

into run-time parameters

debug_copy_parse_plan_trees
debug_write_read_parse_plan_trees
debug_raw_expression_coverage_test

They can be activated for tests using PG_TEST_INITDB_EXTRA_OPTS.

The compile-time symbols are kept for build farm compatibility, but
they now just determine the default value of the run-time settings.

Furthermore, support for these settings is not compiled in at all
unless assertions are enabled, or the new symbol
DEBUG_NODE_TESTS_ENABLED is defined at compile time, or any of the
legacy compile-time setting symbols are defined.  So there is no
run-time overhead in production builds.  (This is similar to the
handling of DISCARD_CACHES_ENABLED.)

Discussion: 
https://www.postgresql.org/message-id/flat/30747bd8-f51e-4e0c-a310-a6e2c37ec8aa%40eisentraut.org
---
 .cirrus.tasks.yml   |  3 +-
 doc/src/sgml/config.sgml| 71 +
 src/backend/nodes/README|  9 ++--
 src/backend/nodes/read.c| 12 ++---
 src/backend/nodes/readfuncs.c   |  4 +-
 src/backend/parser/analyze.c| 44 ++
 src/backend/tcop/postgres.c | 30 +++-
 src/backend/utils/misc/guc_tables.c | 59 
 src/include/nodes/nodes.h   |  2 +-
 src/include/nodes/readfuncs.h   |  2 +-
 src/include/pg_config_manual.h  | 34 +++---
 src/include/utils/guc.h |  6 +++
 12 files changed, 212 insertions(+), 64 deletions(-)

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index a2388cd5036..6a9ff066391 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -133,9 +133,10 @@ task:
 DISK_SIZE: 50
 
 CCACHE_DIR: /tmp/ccache_dir
-CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES 
-DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST 
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+CPPFLAGS: -DRELCACHE_FORCE_RELEASE 
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
 CFLAGS: -Og -ggdb
 
+PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c 
debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on
 PG_TEST_PG_UPGRADE_MODE: --link
 
   <<: *freebsd_task_template
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 698169afdb6..c121a13b4c3 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11372,6 +11372,29 @@ Developer Options
   
  
 
+ 
+  debug_copy_parse_plan_trees 
(boolean)
+  
+   debug_copy_parse_plan_trees configuration 
parameter
+  
+  
+  
+   
+Enabling this forces all parse and plan trees to be passed through
+copyObject(), to facilitate catching errors and
+omissions in copyObject().  The default is off.
+   
+

Re: speed up a logical replica setup

[Shlok Kyal]

On Wed, 22 May 2024 at 16:50, Amit Kapila  wrote:
>
> On Mon, May 20, 2024 at 4:30 PM Shlok Kyal  wrote:
> > >
> > > I was trying to test this utility when 'sync_replication_slots' is on
> > > and it gets in an ERROR loop [1] and never finishes. Please find the
> > > postgresql.auto used on the standby attached. I think if the standby
> > > has enabled sync_slots, you need to pass dbname in
> > > GenerateRecoveryConfig(). I couldn't test it further but I wonder if
> > > there are already synced slots on the standby (either due to
> > > 'sync_replication_slots' or users have used
> > > pg_sync_replication_slots() before invoking pg_createsubscriber),
> > > those would be retained as it is on new subscriber and lead to
> > > unnecessary WAL retention and dead rows.
> > >
> > > [1]
> > > 2024-04-30 11:50:43.239 IST [12536] LOG:  slot sync worker started
> > > 2024-04-30 11:50:43.247 IST [12536] ERROR:  slot synchronization
> > > requires dbname to be specified in primary_conninfo
> >
> > Hi,
> >
> > I tested the scenario posted by Amit in [1], in which retaining synced
> > slots lead to unnecessary WAL retention and ERROR. This is raised as
> > the second open point in [2].
> > The steps to reproduce the issue:
> > (1) Setup physical replication with sync slot feature turned on by
> > setting sync_replication_slots = 'true' or using
> > pg_sync_replication_slots() on the standby node.
> > For physical replication setup, run pg_basebackup with -R  and -d option.
> > (2) Create a logical replication slot on primary node with failover
> > option as true. A corresponding slot is created on standby as part of
> > sync slot feature.
> > (3) Run pg_createsubscriber on standby node.
> > (4) On Checking for the replication slot on standby node, I noticed
> > that the logical slots created in step 2 are retained.
> >  I have attached the script to reproduce the issue.
> >
> > I and Kuroda-san worked to resolve open points. Here are patches to
> > solve the second and third point in [2].
> > Patches proposed by Euler are also attached just in case, but they
> > were not modified.
> >
> > v2-0001: not changed
> >
>
> Shouldn't we modify it as per the suggestion given in the email [1]? I
> am wondering if we can entirely get rid of checking the primary
> business and simply rely on recovery_timeout and keep checking
> server_is_in_recovery(). If so, we can modify the test to use
> non-default recovery_timeout (say 180s or something similar if we have
> used it at any other place). As an additional check we can ensure that
> constent_lsn is present on standby.
>

I have made changes as per your suggestion. I have used
pg_last_wal_receive_lsn to get lsn last wal received on standby and
check if consistent_lsn is already present on the standby.
I have only made changes in v3-0001. v3-0002, v3-0003, v3-0004 and
v3-0005 are not modified.

Thanks and Regards,
Shlok Kyal


v3-0005-Avoid-outputing-some-messages-in-dry_run-mode.patch
Description: Binary data


v3-0003-Disable-slot-sync-during-the-convertion.patch
Description: Binary data


v3-0001-Improve-the-code-that-checks-if-the-recovery-is-f.patch
Description: Binary data


v3-0004-Drop-replication-slots-which-had-been-synchronize.patch
Description: Binary data


v3-0002-Improve-the-code-that-checks-if-the-primary-slot-.patch
Description: Binary data

Possible incorrect row estimation for Gather paths

[Anthonin Bonnefoy]

Hi,

While experimenting on an explain option to display all plan candidates
(very rough prototype here [1]), I've noticed some discrepancies in some
generated plans.

EXPLAIN (ALL_CANDIDATES) SELECT * FROM pgbench_accounts order by aid;
 Plan 1
->  Gather Merge  (cost=11108.32..22505.38 rows=10 width=97)
 Workers Planned: 1
 ->  Sort  (cost=10108.31..10255.37 rows=58824 width=97)
   Sort Key: aid
   ->  Parallel Seq Scan on pgbench_accounts  (cost=0.00..2228.24
rows=58824 width=97)
 Plan 2
->  Gather Merge  (cost=11108.32..17873.08 rows=58824 width=97)
 Workers Planned: 1
 ->  Sort  (cost=10108.31..10255.37 rows=58824 width=97)
   Sort Key: aid
   ->  Parallel Seq Scan on pgbench_accounts  (cost=0.00..2228.24
rows=58824 width=97)

The 2 plans are similar except one Gather Merge has a lower 58K estimated
rows.

The first plan is created with generate_useful_gather_paths with
override_rows=false then create_gather_merge_path and will use rel->rows as
the row count (so the 100K rows of pgbench_accounts).
#0: create_gather_merge_path(...) at pathnode.c:1885:30
#1: generate_useful_gather_paths(... override_rows=false) at
allpaths.c:3286:11
#2: apply_scanjoin_target_to_paths(...) at planner.c:7744:3
#3: grouping_planner(...) at planner.c:1638:3

The second plan is created through create_ordered_paths then
create_gather_merge_path and the number of rows is estimated to
(worker_rows * parallel_workers). Since we only have 1 parallel worker,
this yields 58K rows.
#0: create_gather_merge_path(...) at pathnode.c:1885:30
#1: create_ordered_paths(...) at planner.c:5287:5
#2: grouping_planner(...) at planner.c:1717:17

The 58K row estimate looks possibly incorrect. A worker row count is
estimated using total_rows/parallel_divisor. The parallel_divisor will
include the possible leader participation and will be 1.7 for 1 worker thus
the 58K rows (100K/1.7=58K)
However, the gather merge will only do 58K*1, dropping the leader
participation component.

I have a tentative patch split in two changes:
1: This is a related refactoring to remove an unnecessary and confusing
assignment of rows in create_gather_merge_path. This value is never used
and immediately overwritten in cost_gather_merge
2: This changes the row estimation of gather path to use
worker_rows*parallel_divisor to get a more accurate estimation.
Additionally, when creating a gather merge path in create_ordered_paths, we
try to use the source's rel rows when available.

The patch triggered a small change in the hash_join regression test. Pre
patch, we had the following candidates.
Plan 4
->  Aggregate  (cost=511.01..511.02 rows=1 width=8)
 ->  Gather  (cost=167.02..511.00 rows=2 width=0)
   Workers Planned: 1
   ->  Parallel Hash Join  (cost=167.02..510.80 rows=1 width=0)
 Hash Cond: (r.id = s.id)
 ->  Parallel Seq Scan on simple r  (cost=0.00..299.65
rows=11765 width=4)
 ->  Parallel Hash  (cost=167.01..167.01 rows=1 width=4)
   ->  Parallel Seq Scan on extremely_skewed s
 (cost=0.00..167.01 rows=1 width=4)
Plan 5
->  Finalize Aggregate  (cost=510.92..510.93 rows=1 width=8)
 ->  Gather  (cost=510.80..510.91 rows=1 width=8)
   Workers Planned: 1
   ->  Partial Aggregate  (cost=510.80..510.81 rows=1 width=8)
 ->  Parallel Hash Join  (cost=167.02..510.80 rows=1
width=0)
   Hash Cond: (r.id = s.id)
   ->  Parallel Seq Scan on simple r
 (cost=0.00..299.65 rows=11765 width=4)
   ->  Parallel Hash  (cost=167.01..167.01 rows=1
width=4)
 ->  Parallel Seq Scan on extremely_skewed s
 (cost=0.00..167.01 rows=1 width=4)

Post patch, the plan candidates became:
Plan 4
->  Finalize Aggregate  (cost=511.02..511.03 rows=1 width=8)
 ->  Gather  (cost=510.80..511.01 rows=2 width=8)
   Workers Planned: 1
   ->  Partial Aggregate  (cost=510.80..510.81 rows=1 width=8)
 ->  Parallel Hash Join  (cost=167.02..510.80 rows=1
width=0)
   Hash Cond: (r.id = s.id)
   ->  Parallel Seq Scan on simple r
 (cost=0.00..299.65 rows=11765 width=4)
   ->  Parallel Hash  (cost=167.01..167.01 rows=1
width=4)
 ->  Parallel Seq Scan on extremely_skewed s
 (cost=0.00..167.01 rows=1 width=4)
Plan 5
->  Aggregate  (cost=511.01..511.02 rows=1 width=8)
 ->  Gather  (cost=167.02..511.00 rows=2 width=0)
   Workers Planned: 1
   ->  Parallel Hash Join  (cost=167.02..510.80 rows=1 width=0)
 Hash Cond: (r.id = s.id)
 ->  Parallel Seq Scan on simple r  (cost=0.00..299.65
rows=11765 width=4)
 ->  Parallel Hash  (cost=167.01..167.01 rows=1 width=4)
   ->  Parallel Seq Scan on extremely_skewed s
 (cost=0.00..167.01 rows=1 width=4)

The FinalizeAggreg

Re: CREATE INDEX CONCURRENTLY on partitioned index

[Alexander Pyhalov]

Ilya Gladyshev писал(а) 2024-05-24 00:14:

Hi,


Hi.



I think it's well worth the effort to revive the patch, so I rebased it 
on master, updated it and will return it back to the commitfest. 
Alexander, Justin feel free to add yourselves as authors


On 29.01.2024 12:43, Alexander Pyhalov wrote:

Hi.

I've rebased patch on master and it'seems to me there's one more issue 
-


when we call DefineIndexConcurrentInternal() in partitioned case, it 
waits for transactions, locking tableId, not tabrelid - heaprelid 
LockRelId is constructed for parent index relation, not for child 
index relation.


Attaching fixed version.

Also I'm not sure what to do with locking of child relations. If we 
don't do anything, you can drop one of the partitioned table childs 
while CIC is in progress, and get error


ERROR:  cache lookup failed for index 16399
I agree that we need to do something about it, in particular, I think 
we should lock all the partitions inside the transaction that builds 
the catalog entries. Fixed this in the new version.
If you try to lock all child tables in CIC session, you'll get 
deadlocks.


Do you mean the deadlock between the transaction that drops a partition 
and the transaction doing CIC? I think this is unavoidable and can be 
reproduced even without partitioning.


Yes, it seems we trade this error for possible deadlock between 
transaction, dropping a partition, and CIC.




Also not sure why a list of children relation was obtained with 
ShareLock that CIC is supposed to avoid not to block writes, changed 
that to ShareUpdateExclusive.




I expect that it wasn't an issue due to the fact that it's held for a 
brief period until DefineIndexConcurrentInternal() commits for the first 
time. But it seems, it's more correct to use ShareUpdateExclusive lock 
here.



Also I'd like to note that in new patch version there's a strange 
wording in documentation:


"This can be very convenient as not only will all existing partitions be
 indexed, but any future partitions will be as well.
 CREATE INDEX ... CONCURRENTLY can incur long lock 
times

 on huge partitioned tables, to avoid that you can
 use CREATE INDEX ON ONLY the partitioned table, 
which
 creates the new index marked as invalid, preventing automatic 
application

 to existing partitions."

All the point of CIC is to avoid long lock times. So it seems this 
paragraph should be rewritten in the following way:


"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
CREATE INDEX ON ONLY the partitioned table..."



--
Best regards,
Alexander Pyhalov,
Postgres Professional

Re: Addressing SECURITY DEFINER Function Vulnerabilities in PostgreSQL Extensions

[Ashutosh Bapat]

On Fri, May 24, 2024 at 12:52 PM Ashutosh Sharma 
wrote:

> Hi All,
>
> We all know that installing an extension typically requires superuser
> privileges, which means the database objects it creates are owned by the
> superuser.
>
> If the extension creates any SECURITY DEFINER functions, it can introduce
> security vulnerabilities. For example, consider an extension that creates
> the following functions, outer_func and inner_func, in the schema s1 when
> installed:
>
> CREATE OR REPLACE FUNCTION s1.inner_func(data text)
> RETURNS void AS $$
> BEGIN
> INSERT INTO tab1(data_column) VALUES (data);
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE OR REPLACE FUNCTION s1.outer_func(data text)
> RETURNS void AS $$
> BEGIN
> PERFORM inner_func(data);
> END;
> $$ LANGUAGE plpgsql SECURITY DEFINER;
>
> If a regular user creates another function with name "inner_func" with the
> same signature in the public schema and sets the search path to public, s1,
> the function created by the regular user in the public schema takes
> precedence when outer_func is called. Since outer_func is marked as
> SECURITY DEFINER, the inner_func created by the user in the public schema
> is executed with superuser privileges. This allows the execution of any
> statements within the function block, leading to potential security issues.
>
> To address this problem, one potential solution is to adjust the function
> resolution logic. For instance, if the caller function belongs to a
> specific schema, functions in the same schema should be given preference.
> Although I haven’t reviewed the feasibility in the code, this is one
> possible approach.
>
> Another solution could be to categorize extension-created functions to
> avoid duplication. This might not be an ideal solution, but it's another
> consideration worth sharing.
>
>
Function call should schema qualify it. That's painful, but it can be
avoided by setting a search path from inside the function. There was some
discussion about setting a search path for a function at [1]. But the last
message there is non-conclusive. We may want to extend it to extensions
such that all the object references in a given extension are resolved using
extension specific search_path.

[1]
https://www.postgresql.org/message-id/2710f56add351a1ed553efb677408e51b060e67c.camel%40j-davis.com

-- 
Best Wishes,
Ashutosh Bapat

Addressing SECURITY DEFINER Function Vulnerabilities in PostgreSQL Extensions

[Ashutosh Sharma]

Hi All,

We all know that installing an extension typically requires superuser
privileges, which means the database objects it creates are owned by the
superuser.

If the extension creates any SECURITY DEFINER functions, it can introduce
security vulnerabilities. For example, consider an extension that creates
the following functions, outer_func and inner_func, in the schema s1 when
installed:

CREATE OR REPLACE FUNCTION s1.inner_func(data text)
RETURNS void AS $$
BEGIN
INSERT INTO tab1(data_column) VALUES (data);
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION s1.outer_func(data text)
RETURNS void AS $$
BEGIN
PERFORM inner_func(data);
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;

If a regular user creates another function with name "inner_func" with the
same signature in the public schema and sets the search path to public, s1,
the function created by the regular user in the public schema takes
precedence when outer_func is called. Since outer_func is marked as
SECURITY DEFINER, the inner_func created by the user in the public schema
is executed with superuser privileges. This allows the execution of any
statements within the function block, leading to potential security issues.

To address this problem, one potential solution is to adjust the function
resolution logic. For instance, if the caller function belongs to a
specific schema, functions in the same schema should be given preference.
Although I haven’t reviewed the feasibility in the code, this is one
possible approach.

Another solution could be to categorize extension-created functions to
avoid duplication. This might not be an ideal solution, but it's another
consideration worth sharing.

Thoughts?

--
With Regards,
Ashutosh Sharma.

PG catalog

[Karki, Sanjay]

Hi ,

I need to grant select on privilege in pg_catalog to user so I can connect via 
Toad Data point ,

I tried by
grant select on all tables in schema pg_catalog to group sys;
while connecting as sys.

But it throws me error

grant select on all tables in schema pg_catalog to  sys ;

ERROR: permission denied for table pg_statistic SQL state: 42501


Can I get some help please


Thank you

Sanjay karki



Sanjay Karki

Database Administrator III

ITG

[cid:image001.png@HNMW31RS4NU4.A9W7106ZTAI5]

O: 816-783-8718
M: 816-394-4383
W: www.naic.org

Follow the NAIC on
[cid:image002.png@HNMW31RS4NU4.A9W7106ZTAI5]
 [cid:image003.png@HNMW31RS4NU4.A9W7106ZTAI5]   
[cid:image004.png@HNMW31RS4NU4.A9W7106ZTAI5] 
  
[cid:image005.png@HNMW31RS4NU4.A9W7106ZTAI5] 




--

CONFIDENTIALITY NOTICE

--

This message and any attachments are from the NAIC and are intended only for 
the addressee. Information contained herein is confidential, and may be 
privileged or exempt from disclosure pursuant to applicable federal or state 
law. This message is not intended as a waiver of the confidential, privileged 
or exempted status of the information transmitted. Unauthorized forwarding, 
printing, copying, distribution or use of such information is strictly 
prohibited and may be unlawful. If you are not the addressee, please promptly 
delete this message and notify the sender of the delivery error by e-mail or by 
forwarding it to the NAIC Service Desk at h...@naic.org.

Re: In-placre persistance change of a relation

[Kyotaro Horiguchi]

Rebased.

Along with rebasing, I changed the interface of XLogFsyncFile() to
return a boolean instead of an error message.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From bed74e638643d7491bbd86fe640c33db1e16f0e5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 15 Jan 2024 15:57:53 +0900
Subject: [PATCH v33 1/3] Export wal_sync_method related functions

Export several functions related to wal_sync_method for use in
subsequent commits. Since PG_O_DIRECT cannot be used in those commits,
the new function XLogGetSyncBit() will mask PG_O_DIRECT.
---
 src/backend/access/transam/xlog.c | 73 +--
 src/include/access/xlog.h |  2 +
 2 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 330e058c5f..492ababd9c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8592,21 +8592,29 @@ assign_wal_sync_method(int new_wal_sync_method, void *extra)
 	}
 }
 
+/*
+ * Exported version of get_sync_bit()
+ *
+ * Do not expose PG_O_DIRECT for uses outside xlog.c.
+ */
+int
+XLogGetSyncBit(void)
+{
+	return get_sync_bit(wal_sync_method) & ~PG_O_DIRECT;
+}
+
 
 /*
- * Issue appropriate kind of fsync (if any) for an XLOG output file.
+ * Issue appropriate kind of fsync (if any) according to wal_sync_method.
  *
- * 'fd' is a file descriptor for the XLOG file to be fsync'd.
- * 'segno' is for error reporting purposes.
+ * 'fd' is a file descriptor for the file to be fsync'd.
  */
-void
-issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
+const char *
+XLogFsyncFile(int fd)
 {
-	char	   *msg = NULL;
+	const char *msg = NULL;
 	instr_time	start;
 
-	Assert(tli != 0);
-
 	/*
 	 * Quick exit if fsync is disabled or write() has already synced the WAL
 	 * file.
@@ -8614,7 +8622,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
 	if (!enableFsync ||
 		wal_sync_method == WAL_SYNC_METHOD_OPEN ||
 		wal_sync_method == WAL_SYNC_METHOD_OPEN_DSYNC)
-		return;
+		return NULL;
 
 	/* Measure I/O timing to sync the WAL file */
 	if (track_wal_io_timing)
@@ -8651,19 +8659,6 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
 			break;
 	}
 
-	/* PANIC if failed to fsync */
-	if (msg)
-	{
-		char		xlogfname[MAXFNAMELEN];
-		int			save_errno = errno;
-
-		XLogFileName(xlogfname, tli, segno, wal_segment_size);
-		errno = save_errno;
-		ereport(PANIC,
-(errcode_for_file_access(),
- errmsg(msg, xlogfname)));
-	}
-
 	pgstat_report_wait_end();
 
 	/*
@@ -8677,7 +8672,39 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
 		INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, end, start);
 	}
 
-	PendingWalStats.wal_sync++;
+	if (msg != NULL)
+		PendingWalStats.wal_sync++;
+
+	return msg;
+}
+
+/*
+ * Issue appropriate kind of fsync (if any) for an XLOG output file.
+ *
+ * 'fd' is a file descriptor for the XLOG file to be fsync'd.
+ * 'segno' is for error reporting purposes.
+ */
+void
+issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
+{
+	const char	*msg;
+	
+	Assert(tli != 0);
+
+	msg = XLogFsyncFile(fd);
+
+	/* PANIC if failed to fsync */
+	if (msg)
+	{
+		char		xlogfname[MAXFNAMELEN];
+		int			save_errno = errno;
+
+		XLogFileName(xlogfname, tli, segno, wal_segment_size);
+		errno = save_errno;
+		ereport(PANIC,
+(errcode_for_file_access(),
+ errmsg(msg, xlogfname)));
+	}
 }
 
 /*
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 1a1f11a943..badfe4abd6 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -217,6 +217,8 @@ extern void xlog_redo(struct XLogReaderState *record);
 extern void xlog_desc(StringInfo buf, struct XLogReaderState *record);
 extern const char *xlog_identify(uint8 info);
 
+extern int XLogGetSyncBit(void);
+extern const char *XLogFsyncFile(int fd);
 extern void issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli);
 
 extern bool RecoveryInProgress(void);
-- 
2.43.0

>From c200b85c1311f97bdae2ed20e2746c44d5c4aadb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 31 Aug 2023 11:49:10 +0900
Subject: [PATCH v33 2/3] Introduce undo log implementation

This patch adds a simple implementation of UNDO log feature.
---
 src/backend/access/transam/Makefile|   1 +
 src/backend/access/transam/meson.build |   1 +
 src/backend/access/transam/rmgr.c  |   4 +-
 src/backend/access/transam/simpleundolog.c | 362 +
 src/backend/access/transam/twophase.c  |   3 +
 src/backend/access/transam/xact.c  |  24 ++
 src/backend/access/transam/xlog.c  |  42 ++-
 src/backend/catalog/storage.c  | 171 ++
 src/backend/storage/file/reinit.c  |  78 +
 src/backend/storage/smgr/smgr.c|   9 +
 src/bin/initdb/initdb.c|  17 +
 src/bin/pg_rewind/parsexlog.c  |   2 +-
 src/bin/pg_waldump/rmgrdesc.c  |   2 +-
 src/in