Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

Hi,

On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > I also don’t think that I agree that it’s acceptable to only have the
> > > ability to extend the authentication on the server side as that implies a
> > > whole bunch of client-side work that goes above and beyond just
> > > implementing an authentication system if it’s not possible to leverage
> > > libpq (which nearly all languages out there use..).  Not addressing that
> > > side of it also makes me concerned that whatever we do here won’t be
> > > right/enough/etc.
> >
> > You're skipping over my point of everything that can be made to use
> > SASL-SCRAM-256 just working with the existing libpq? Again?

> The plan is to make scram pluggable on the client side?  That isn’t what’s
> been proposed here that I’ve seen.

Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
implementing an authentication method that wants to store secrets outside of
postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
don't need to make scram pluggable client-side. From the perspective of the
client it'd look *exactly* like the normal auth flow with the server.

What's proposed here is not really about adding new authentication protocols
between FE/BE (although some *limited* forms of that might be possible). It's
primarily about using the existing FE/BE authentication exchanges
(AUTH_REQ_{PASSWORD,SASL*,...}) to validate against something other than the
builtin pg_authid.rolpassword.  Because drivers already know those
authentication exchanges, it doesn't need to learn new tricks.

As I said in
https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
when describing the above, that's not enough to implement every desireable
authentication method - but there's real use-cases where using the existing
SASL-SCRAM-256 is sufficient.


> I also wasn’t aware that we felt that it’s acceptable to just ignore other
> committers.

I'm not talking about going ahead and committing. Just not continuing
discussing this topci and spending my time more fruitfully on something else.

Greetings,

Andres Freund

Re: Column Filtering in Logical Replication

[Amit Kapila]

On Fri, Mar 18, 2022 at 12:47 AM Tomas Vondra
 wrote:
>
> I pushed the second fix. Interestingly enough, wrasse failed in the
> 013_partition test. I don't see how that could be caused by this
> particular commit, though - see the pgsql-committers thread [1].
>

I have a theory about what's going on here. I think this is due to a
test added in your previous commit c91f71b9dc. The newly added test
added hangs in tablesync because there was no apply worker to set the
state to SUBREL_STATE_CATCHUP which blocked tablesync workers from
proceeding.

See below logs from pogona [1].
2022-03-18 01:33:15.190 CET [2551176][client
backend][3/74:0][013_partition.pl] LOG:  statement: ALTER SUBSCRIPTION
sub2 SET PUBLICATION pub_lower_level, pub_all
2022-03-18 01:33:15.354 CET [2551193][logical replication
worker][4/57:0][] LOG:  logical replication apply worker for
subscription "sub2" has started
2022-03-18 01:33:15.605 CET [2551176][client
backend][:0][013_partition.pl] LOG:  disconnection: session time:
0:00:00.415 user=bf database=postgres host=[local]
2022-03-18 01:33:15.607 CET [2551209][logical replication
worker][3/76:0][] LOG:  logical replication table synchronization
worker for subscription "sub2", table "tab4_1" has started
2022-03-18 01:33:15.609 CET [2551211][logical replication
worker][5/11:0][] LOG:  logical replication table synchronization
worker for subscription "sub2", table "tab3" has started
2022-03-18 01:33:15.617 CET [2551193][logical replication
worker][4/62:0][] LOG:  logical replication apply worker for
subscription "sub2" will restart because of a parameter change

You will notice that the apply worker is never restarted after a
parameter change. The reason was that the particular subscription
reaches the limit of max_sync_workers_per_subscription after which we
don't allow to restart the apply worker. I think you might want to
increase the values of
max_sync_workers_per_subscription/max_logical_replication_workers to
make it work.

> I'd like to test & polish the main patch over the weekend, and get it
> committed early next week. Unless someone thinks it's definitely not
> ready for that ...
>

I think it is in good shape but apart from cleanup, there are issues
with dependency handling which I have analyzed and reported as one of
the comments in the email [2]. I was getting some weird behavior
during my testing due to that. Apart from that still the patch has DDL
handling code in tablecmds.c which probably is not required.
Similarly, Shi-San has reported an issue with replica full in her
email [3]. It is up to you what to do here but it would be good if you
can once share the patch after fixing these issues so that we can
re-test/review it.


[1] - 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pogona=2022-03-17%2023%3A10%3A04
[2] - 
https://www.postgresql.org/message-id/CAA4eK1KR%2ByUQquK0Bx9uO3eb5xB1e0rAD9xKf-ddm5nSf4WfNg%40mail.gmail.com
[3] - 
https://www.postgresql.org/message-id/TYAPR01MB6315D664D926EF66DD6E91FCFD109%40TYAPR01MB6315.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

[Bharath Rupireddy]

On Fri, Mar 18, 2022 at 2:15 AM Nathan Bossart  wrote:
>
> On Thu, Mar 17, 2022 at 06:48:43PM +0530, Bharath Rupireddy wrote:
> > Personally, I tend to agree with v4-0001 (option (4)) or v4-0002
> > (option (3)) than v4-0003 (option (1)) as it adds more unreadability
> > with most of the code duplicated creating more diff with previous
> > versions and maintainability problems. Having said that, I will leave
> > it to the committer to decide on that.
>
> I don't think v4-0003/option 1 needs to be unreadable.  Perhaps we can use
> a StringInfo to build the message.  That might be a net improvement in
> readability anyway.

Looks like we already use StringInfo in PerformAuthentication for
"connection" related log messages, see [1].

I will give it a try using StringInfo for the "checkpoint/restartpoint
complete" message too.

[1]
if (Log_connections)
{
StringInfoData logmsg;

initStringInfo();
if (am_walsender)
appendStringInfo(, _("replication connection
authorized: user=%s"),
 port->user_name);
else
appendStringInfo(, _("connection authorized: user=%s"),
 port->user_name);
if (!am_walsender)
appendStringInfo(, _(" database=%s"), port->database_name);

Regards,
Bharath Rupireddy.

RE: Logical replication timeout problem

[wangw.f...@fujitsu.com]

On Thu, Mar 9, 2022 at 11:52 AM Kuroda, Hayato/黒田 隼人 
 wrote:
> Thank you for updating!
Thanks for your comments.

> 1. pgoutput_change
> ```
> +   bool is_send = true;
> ```
> 
> My first impression is that is_send should be initialized to false, and it 
> will change
> to true when OutputPluginWrite() is called.
> 
> 
> 2. pgoutput_change
> ```
> +   {
> +   is_send = false;
> +   break;
> +   }
> ```
> 
> Here are too many indents, but I think they should be removed.
> See above comment.
Fixed. Initialize is_send to false.

> 3. WalSndUpdateProgress
> ```
> +   /*
> +* If half of wal_sender_timeout has lapsed without send 
> message
> standby,
> +* send a keep-alive message to the standby.
> +*/
> ```
> 
> The comment seems inconsistency with others.
> Here is "keep-alive", but other parts are "keepalive".
Since this part of the code was refactored, this inconsistent comment was
removed.

> 4. ReorderBufferProcessTXN
> ```
> + 
>   change-
> >data.inval.ninvalidations,
> +
> + change->data.inval.invalidations);
> ```
> 
> Maybe these lines break 80-columns rule.
Thanks for reminder. I will run pg_ident later.

Kindly have a look at new patch shared in [1].

[1] - 
https://www.postgresql.org/message-id/OS3PR01MB6275C67F14954E05CE5D04399E139%40OS3PR01MB6275.jpnprd01.prod.outlook.com

Regards,
Wang wei

RE: Logical replication timeout problem

[wangw.f...@fujitsu.com]

On Thu, Mar 17, 2022 at 7:52 PM Masahiko Sawada  wrote:
>
Thanks for your comments.

> On Thu, Mar 17, 2022 at 7:14 PM Amit Kapila  wrote:
> >
> > On Thu, Mar 17, 2022 at 12:27 PM Amit Kapila 
> wrote:
> > >
> > > On Wed, Mar 16, 2022 at 7:38 PM Masahiko Sawada
>  wrote:
> > > >
> > > > After more thought, can we check only wal_sender_timeout without
> > > > skip-count? That is, in WalSndUpdateProgress(), if we have
> > > > received any reply from the subscriber in last (wal_sender_timeout
> > > > / 2), we don't need to do anything in terms of keep-alive. If not,
> > > > we do
> > > > ProcessRepliesIfAny() (and probably WalSndCheckTimeOut()?) then
> > > > WalSndKeepalivesIfNecessary(). That way, we can send keep-alive
> > > > messages every (wal_sender_timeout / 2). And since we don't call
> > > > them for every change, we would not need to worry about the overhead
> much.
> > > >
> > >
> > > But won't that lead to a call to GetCurrentTimestamp() for each
> > > change we skip? IIUC from previous replies that lead to a slight
> > > slowdown in previous tests of Wang-San.
> > >
> > If the above is true then I think we can use a lower skip_count say 10
> > along with a timeout mechanism to send keepalive message. This will
> > help us to alleviate the overhead Wang-San has shown.
> 
> Using both sounds reasonable to me. I'd like to see how much the overhead is
> alleviated by using skip_count 10 (or 100).
> 
> > BTW, I think there could be one other advantage of using
> > ProcessRepliesIfAny() (as you are suggesting) is that it can help to
> > release sync waiters if there are any. I feel that would be the case
> > for the skip_empty_transactions patch [1] which uses
> > WalSndUpdateProgress to send keepalive messages after skipping empty
> > transactions.
> 
> +1
I modified the patch according to your and Amit-San's suggestions.
In addition, after testing, I found that when the threshold is 10, it brings
slight overhead.
So I try to change it to 100, after testing, the results look good to me.
10  : 1.22%--UpdateProgress
100 : 0.16%--UpdateProgress

Please refer to attachment.

Attach the new patch.
1. Refactor the way to send keepalive messages.
   [suggestion by Sawada-San, Amit-San.]
2. Modify the value of flag is_send initialization to make it look more
   reasonable. [suggestion by Kuroda-San.]
3. Improve new function names.
   (From SendKeepaliveIfNecessary to UpdateProgress.)

Regards,
Wang wei


v3-0001-Fix-the-timeout-of-subscriber-in-long-transaction.patch
Description:  v3-0001-Fix-the-timeout-of-subscriber-in-long-transaction.patch

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

On Fri, Mar 18, 2022 at 00:24 Andres Freund  wrote:

> Hi,
>
> On 2022-03-18 00:03:37 -0400, Stephen Frost wrote:
> > On Thu, Mar 17, 2022 at 23:25 Andres Freund  wrote:
> > > It's imo a separate project to make the client side extensible. There's
> > > plenty
> > > of authentication methods that don't need any further client side
> support
> > > than
> > > the existing SASL (or password if OK for some use case) messages, which
> > > most
> > > clients (including libpq) already know.
> > >
> > > Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit
> > > (unless
> > > you have server side access to clear text passwords, but brrr). But
> there's
> > > plenty that can be done with that. Proxying auth via a central postgres
> > > server
> > > with the secrets, sharing secrets with other data stores also
> understanding
> > > SCRAM-SHA-256, ...
> > >
> > > There definitely *also* are important authentication methods that
> can't be
> > > implemented without further client side support. Some of those could
> > > plausibly
> > > be tackled on the protocol / libpq level in a way that they could be
> used
> > > by
> > > multiple authentication methods. Other authentication methods
> definitely
> > > need
> > > dedicated code in the client (although the protocol likely can be
> fairly
> > > generic).
> > >
> > > Given that there's benefit from the server side extensibility alone, I
> > > don't
> > > see much benefit in tying the server side extensibility to the client
> side
> > > extensibility.
> >
> >
> > How are we going to reasonably test this then?
>
> The test module in the patch is a good starting point.


Without a complete, independent, “this is how it will really be used” on
both the server and client side set of tests I’m not sure that it is.

> I also don’t think that I agree that it’s acceptable to only have the
> > ability to extend the authentication on the server side as that implies a
> > whole bunch of client-side work that goes above and beyond just
> > implementing an authentication system if it’s not possible to leverage
> > libpq (which nearly all languages out there use..).  Not addressing that
> > side of it also makes me concerned that whatever we do here won’t be
> > right/enough/etc.
>
> You're skipping over my point of everything that can be made to use
> SASL-SCRAM-256 just working with the existing libpq? Again?


The plan is to make scram pluggable on the client side?  That isn’t what’s
been proposed here that I’ve seen.  Or is the idea that we are going to
have built-in support for some subset of “custom” things, but only on the
client side because it’s hard to do custom things there, but they’re custom
and have to be loaded through shared preload libraries on the server side?

If you want to argue that somehow that communicating via SASL-SCRAM-256
> from a
> plugin is not a sufficient use-case, or that the API should be shaped more
> specifically to just use SASL-SCRAM-256, fine. Argue it. Otherwise I don't
> know what to do except ignore you.


One thrust of my argument is that if we are going to support custom
authentication methods then we need to consider both sides of that, not
just the server side.  Another is- it’s highly unlikely that the next
authentication method will actually be able to be implemented using these
custom hooks, based on the history we have seen of pluggable authentication
systems, and therefore I don’t agree that they make sense or will be what
we need in the future, which seems to be a large thrust of the argument
here.  I’m also concerned about the risks that this presents to the project
in that there will be arguments about where the fault lies between the
hooks and the core code, as this is just inherently security-related bits,
yet that doesn’t seem to be addressed.  Either way though, it strikes me as
likely to be leaving our users in a bad position either way.

>
I also wasn’t aware that we felt that it’s acceptable to just ignore other
committers.

Thanks,

Stephen

>

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Fri, Mar 18, 2022 at 1:44 AM Robert Haas  wrote:
>
> On Wed, Mar 16, 2022 at 12:53 AM Dilip Kumar  wrote:
> > Thanks Ashutosh and Robert.  Other pacthes cleanly applied on this
> > patch still generated a new version so that we can find all patches
> > together.  There are no other changes.
>
> I committed my v3 of my refactoring patch, here 0001.
>
> I'm working over the comments in the rest of the patch series and will
> post an updated version when I get done. I think I will likely merge
> all the remaining patches together just to make it simpler to manage;
> we can split things out again if we need to do that.

Thanks for the effort.

> One question that occurred to me when looking this over is whether, or
> why, it's safe against concurrent smgr invalidations.

We are only accessing the smgr of the source database and the
destination database.  And there is no one else that can be connected
to the source db and the destination db is not visible to anyone.  So
do we really need to worry about the concurrent smgr invalidation?
What am I missing?

It seems to me
> that every loop in the new CREATE DATABASE code needs to
> CHECK_FOR_INTERRUPTS() -- some do already -- and when they do that,

Yes, the pg_class reading code is missing this check so we need to put
it.  But copying code like
CreateDatabaseUsingWalLog() have it inside the deepest loop in
RelationCopyStorageUsingBuffer() and similarly
CreateDatabaseUsingFileCopy() have it in copydir().  Maybe we should
put it in all loop so that we do not skip checking due to some
condition.

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

Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

Hi,

On 2022-03-18 00:03:37 -0400, Stephen Frost wrote:
> On Thu, Mar 17, 2022 at 23:25 Andres Freund  wrote:
> > It's imo a separate project to make the client side extensible. There's
> > plenty
> > of authentication methods that don't need any further client side support
> > than
> > the existing SASL (or password if OK for some use case) messages, which
> > most
> > clients (including libpq) already know.
> >
> > Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit
> > (unless
> > you have server side access to clear text passwords, but brrr). But there's
> > plenty that can be done with that. Proxying auth via a central postgres
> > server
> > with the secrets, sharing secrets with other data stores also understanding
> > SCRAM-SHA-256, ...
> >
> > There definitely *also* are important authentication methods that can't be
> > implemented without further client side support. Some of those could
> > plausibly
> > be tackled on the protocol / libpq level in a way that they could be used
> > by
> > multiple authentication methods. Other authentication methods definitely
> > need
> > dedicated code in the client (although the protocol likely can be fairly
> > generic).
> >
> > Given that there's benefit from the server side extensibility alone, I
> > don't
> > see much benefit in tying the server side extensibility to the client side
> > extensibility.
> 
> 
> How are we going to reasonably test this then?

The test module in the patch is a good starting point.


> I also don’t think that I agree that it’s acceptable to only have the
> ability to extend the authentication on the server side as that implies a
> whole bunch of client-side work that goes above and beyond just
> implementing an authentication system if it’s not possible to leverage
> libpq (which nearly all languages out there use..).  Not addressing that
> side of it also makes me concerned that whatever we do here won’t be
> right/enough/etc.

You're skipping over my point of everything that can be made to use
SASL-SCRAM-256 just working with the existing libpq? Again?

If you want to argue that somehow that communicating via SASL-SCRAM-256 from a
plugin is not a sufficient use-case, or that the API should be shaped more
specifically to just use SASL-SCRAM-256, fine. Argue it. Otherwise I don't
know what to do except ignore you.

Greetings,

Andres Freund

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

On Thu, Mar 17, 2022 at 23:25 Andres Freund  wrote:

> On 2022-03-17 22:13:27 -0400, Stephen Frost wrote:
> > This isn’t the first time I asked about this on this thread, yet the
> > question about why this is only being discussed as backend changes, and
> > with the only goal seeming to be to have a backend loadable module,
> without
> > anything on the client side for something that’s clearly both a server
> and
> > client side concern, seems to just be ignored, which make me feel like my
> > comments and the concerns that I raise aren’t being appreciated.
>
> It's imo a separate project to make the client side extensible. There's
> plenty
> of authentication methods that don't need any further client side support
> than
> the existing SASL (or password if OK for some use case) messages, which
> most
> clients (including libpq) already know.
>
> Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit
> (unless
> you have server side access to clear text passwords, but brrr). But there's
> plenty that can be done with that. Proxying auth via a central postgres
> server
> with the secrets, sharing secrets with other data stores also understanding
> SCRAM-SHA-256, ...
>
> There definitely *also* are important authentication methods that can't be
> implemented without further client side support. Some of those could
> plausibly
> be tackled on the protocol / libpq level in a way that they could be used
> by
> multiple authentication methods. Other authentication methods definitely
> need
> dedicated code in the client (although the protocol likely can be fairly
> generic).
>
> Given that there's benefit from the server side extensibility alone, I
> don't
> see much benefit in tying the server side extensibility to the client side
> extensibility.


How are we going to reasonably test this then?

I also don’t think that I agree that it’s acceptable to only have the
ability to extend the authentication on the server side as that implies a
whole bunch of client-side work that goes above and beyond just
implementing an authentication system if it’s not possible to leverage
libpq (which nearly all languages out there use..).  Not addressing that
side of it also makes me concerned that whatever we do here won’t be
right/enough/etc.

This is not an area, in my view, where flexibility for the sake of it is
good.  Misunderstandings between the client and the server or between how
the core code and the hooks interact seem very likely and could easily lead
to insecure setups and a good chance for CVEs.

Much like encryption, authentication isn’t easy to get right.  We should be
working to implement standard that experts, through RFCs and similar
well-defined protocols, have defined in the core code with lots of eyes
looking at it.

So, very much a -1 from me for trying to extend the backend in this way.  I
see a lot of risk and not much, if any, benefit.  I’d much rather see us
add support directly in the core code, on the client and sever side, for
OAUTH and other well defined authentication methods, and we even have an
active patch for that could make progress on that with a bit of review.

Thanks,

Stephen

>

Re: Assert in pageinspect with NULL pages

[Alexander Lakhin]

Hello Michael,
18.03.2022 05:06, Michael Paquier wrote:

Okay.  I'll try to look more closely at that, then.  It feels like we
are just missing a MAXALIGN() in those code paths.  Are you using any
specific CFLAGS or something environment-sensitive (macos, 32b, etc.)?
No, just x86_64, Ubuntu 20.04, gcc 11, valgrind 3.15. I just put that 
query in page.sql and see the server abort.


Best regards,
Alexander

Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

Hi,

On 2022-03-17 22:13:27 -0400, Stephen Frost wrote:
> This isn’t the first time I asked about this on this thread, yet the
> question about why this is only being discussed as backend changes, and
> with the only goal seeming to be to have a backend loadable module, without
> anything on the client side for something that’s clearly both a server and
> client side concern, seems to just be ignored, which make me feel like my
> comments and the concerns that I raise aren’t being appreciated.

It's imo a separate project to make the client side extensible. There's plenty
of authentication methods that don't need any further client side support than
the existing SASL (or password if OK for some use case) messages, which most
clients (including libpq) already know.

Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit (unless
you have server side access to clear text passwords, but brrr). But there's
plenty that can be done with that. Proxying auth via a central postgres server
with the secrets, sharing secrets with other data stores also understanding
SCRAM-SHA-256, ...

There definitely *also* are important authentication methods that can't be
implemented without further client side support. Some of those could plausibly
be tackled on the protocol / libpq level in a way that they could be used by
multiple authentication methods. Other authentication methods definitely need
dedicated code in the client (although the protocol likely can be fairly
generic).

Given that there's benefit from the server side extensibility alone, I don't
see much benefit in tying the server side extensibility to the client side
extensibility.

Greetings,

Andres Freund

New Object Access Type hooks

[Mark Dilger]

Hackers,

Over in [1], Joshua proposed a new set of Object Access Type hooks based on 
strings rather than Oids.

His patch was written to be applied atop my patch for granting privileges on 
gucs.

On review of his patch, I became uncomfortable with the complete lack of 
regression test coverage.  To be fair, he did paste a bit of testing logic to 
the thread, but it appears to be based on pgaudit, and it is unclear how to 
include such a test in the core project, where pgaudit is not assumed to be 
installed.

First, I refactored his patch to work against HEAD and not depend on my GUCs 
patch.  Find that as v1-0001.  The refactoring exposed a bit of a problem.  To 
call the new hook for SET and ALTER SYSTEM commands, I need to pass in the Oid 
of a catalog table.  But since my GUC patch isn't applied yet, there isn't any 
such table (pg_setting_acl or whatnot) to pass.  So I'm passing InvalidOid, but 
I don't know if that is right.  In any event, if we want a new API like this, 
we should think a bit harder about whether it can be used to check operations 
where no table Oid is applicable.

Second, I added a new test directory, src/test/modules/test_oat_hooks, which 
includes a new loadable module with hook implementations and a regression test 
for testing the object access hooks.  The main point of the test is to log 
which hooks get called in which order, and which hooks do or do not get called 
when other hooks allow or deny access.  That information shows up in the 
expected output as NOTICE messages.

This second patch has gotten a little long, and I'd like another pair of eyes 
on this before spending a second day on the effort.  Please note that this is a 
quick WIP patch in response to the patch Joshua posted earlier today.  Sorry 
for sometimes missing function comments, etc.  The goal, if this design seems 
acceptable, is to polish this, hopefully with Joshua's assistance, and get it 
committed *before* my GUCs patch, so that my patch can be rebased to use it.  
Otherwise, if this is rejected, I can continue on the GUC patch without this.

(FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when 
testing v1-0001.  I'm not sure yet what that is about.)



v1-0001-Add-String-object-access-hooks.patch
Description: Binary data


v1-0002-Add-regression-tests-of-Object-Access-Type-hooks.patch
Description: Binary data


[1] 
https://www.postgresql.org/message-id/flat/664799.1647456444%40sss.pgh.pa.us#c9721c2da88d59684ac7ac5fc36f09c1

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: XID formatting and SLRU refactorings

[Tom Lane]

Kyotaro Horiguchi  writes:
> At Thu, 17 Mar 2022 19:25:00 +0300, Maxim Orlov  wrote in 
>> +/* printf/elog format compatible with 32 and 64 bit xid. */
>> +typedef unsigned long long  XID_TYPE;
>> ...
>> + errmsg_internal("found multixact %llu from before relminmxid %llu",
>> + (XID_TYPE) multi, (XID_TYPE) relminmxid)));

> "(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
> point here is "%llu in format string accepts (long long)" so we should
> use literally (or bare) (long long) and the maybe-all precedents does
> that.

Yes.  Please do NOT do it like that.  Write (long long), not something
else, to cast a value to match an "ll" format specifier.  Otherwise
you're just making readers wonder whether your code is correct.

regards, tom lane

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

On Thu, Mar 17, 2022 at 17:02 Andres Freund  wrote:

> On 2022-03-16 18:50:23 -0400, Stephen Frost wrote:
> > First, let's be clear- we aren't actually talking about custom or
> > pluggable authentication here, at least when it comes to PG as a
> > project.  For it to actually be pluggable, it needs to be supported on
> > both the client side and the server side, not just the server side.
> >
> > That this keeps getting swept under the carpet makes me feel like this
> > isn't actually an argument about the best way to move the PG project
> > forward but rather has another aim.
>
> This is insulting and unjustified. IMO completely inappropriate for the
> list /
> community. I've also brought this up privately, but I thought it important
> to
> state so publically.


I am concerned.

I don’t intend to insult you or anyone else on this thread.  I’m sorry.

This isn’t the first time I asked about this on this thread, yet the
question about why this is only being discussed as backend changes, and
with the only goal seeming to be to have a backend loadable module, without
anything on the client side for something that’s clearly both a server and
client side concern, seems to just be ignored, which make me feel like my
comments and the concerns that I raise aren’t being appreciated.

I had drafted a response to your private email to me but hadn’t wanted to
send it without going over it again after taking time to be sure I had
cooled down and was being level-headed in my response.

I am sorry.  I am still concerned.

Thanks,

Stephen

>

Re: Assert in pageinspect with NULL pages

[Michael Paquier]

On Wed, Mar 16, 2022 at 08:35:59PM +0300, Alexander Lakhin wrote:
> Yes, I've reproduced that issue on master (46d9bfb0).

Okay.  I'll try to look more closely at that, then.  It feels like we
are just missing a MAXALIGN() in those code paths.  Are you using any
specific CFLAGS or something environment-sensitive (macos, 32b, etc.)?

> pageinspect-zeros-v2.patch doesn't help too.

Well, the scope is different, so that's not a surprise.
--
Michael


signature.asc
Description: PGP signature

Re: Teach pg_receivewal to use lz4 compression

[Michael Paquier]

On Thu, Mar 17, 2022 at 06:12:20AM -0500, Justin Pryzby wrote:
> I think this should use 
> 
> +#include "lz4frame.h"
> 
> commit ba595d2322da095a1e6703171b3f1f2815cb
> Author: Michael Paquier 
> Date:   Fri Nov 5 11:33:25 2021 +0900
> 
> Add support for LZ4 compression in pg_receivewal

Yes, you are right.  A second thing is that should be declared before
the PG headers.
--
Michael


signature.asc
Description: PGP signature

Re: Tab completion for ALTER MATERIALIZED VIEW ... SET ACCESS METHOD

[Yugo NAGATA]

Hi Michael-san,

On Wed, 16 Mar 2022 16:18:09 +0900
Michael Paquier  wrote:

> Hi Nagata-san,
> 
> On Wed, Mar 16, 2022 at 01:33:37PM +0900, Yugo NAGATA wrote:
> > SET ACCESS METHOD is supported in ALTER TABLE since the commit
> > b0483263dd. Since that time,  this also has be allowed SET ACCESS
> > METHOD in ALTER MATERIALIZED VIEW. Although it is not documented,
> > this works.
> 
> Yes, that's an oversight.  I see no reason to not authorize that, and
> the rewrite path in tablecmds.c is the same as for plain tables.
> 
> > I cannot found any reasons to prohibit SET ACCESS METHOD in ALTER
> > MATERIALIZED VIEW, so I think it is better to support this in psql
> > tab-completion and be documented.
> 
> I think that we should have some regression tests about those command
> flavors.  How about adding a couple of queries to create_am.sql for
> SET ACCESS METHOD and to tablespace.sql for SET TABLESPACE?

Thank you for your review!

I added some queries in the regression test. Attached is the updated patch.

Regards,
Yugo Nagata


-- 
Yugo NAGATA 
diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml
index 7011a0e7da..cae135c27a 100644
--- a/doc/src/sgml/ref/alter_materialized_view.sgml
+++ b/doc/src/sgml/ref/alter_materialized_view.sgml
@@ -43,6 +43,8 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE namecolumn_name SET COMPRESSION compression_method
 CLUSTER ON index_name
 SET WITHOUT CLUSTER
+SET ACCESS METHOD new_access_method
+SET TABLESPACE new_tablespace
 SET ( storage_parameter [= value] [, ... ] )
 RESET ( storage_parameter [, ... ] )
 OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 380cbc0b1f..c3fddcd0af 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2124,7 +2124,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("TO");
 	/* ALTER MATERIALIZED VIEW xxx SET */
 	else if (Matches("ALTER", "MATERIALIZED", "VIEW", MatchAny, "SET"))
-		COMPLETE_WITH("(", "SCHEMA", "TABLESPACE", "WITHOUT CLUSTER");
+		COMPLETE_WITH("(", "ACCESS METHOD", "SCHEMA", "TABLESPACE", "WITHOUT CLUSTER");
 	/* ALTER POLICY  */
 	else if (Matches("ALTER", "POLICY"))
 		COMPLETE_WITH_QUERY(Query_for_list_of_policies);
@@ -2485,6 +2485,13 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches("ALTER", "TYPE", MatchAny, "RENAME", "VALUE"))
 		COMPLETE_WITH_ENUM_VALUE(prev3_wd);
 
+	/*
+	 * If we have ALTER MATERIALIZED VIEW  SET ACCESS METHOD provide a list of table
+	 * AMs.
+	 */
+	else if (Matches("ALTER", "MATERIALIZED", "VIEW", MatchAny, "SET", "ACCESS", "METHOD"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_table_access_methods);
+
 /*
  * ANALYZE [ ( option [, ...] ) ] [ table_and_columns [, ...] ]
  * ANALYZE [ VERBOSE ] [ table_and_columns [, ...] ]
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index 32b7134080..cf83c75ab3 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -254,9 +254,33 @@ SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
  9 | 1
 (1 row)
 
+-- ALTER MATERIALIZED VIEW SET ACCESS METHOD
+CREATE MATERIALIZED VIEW heapmv USING heap AS SELECt * FROM heaptable;
+SELECT amname FROM pg_class c, pg_am am
+  WHERE c.relam = am.oid AND c.oid = 'heapmv'::regclass;
+ amname 
+
+ heap
+(1 row)
+
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
+SELECT amname FROM pg_class c, pg_am am
+  WHERE c.relam = am.oid AND c.oid = 'heapmv'::regclass;
+ amname 
+
+ heap2
+(1 row)
+
+SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heapmv;
+ count | count 
+---+---
+ 9 | 1
+(1 row)
+
 -- No support for multiple subcommands
 ALTER TABLE heaptable SET ACCESS METHOD heap, SET ACCESS METHOD heap2;
 ERROR:  cannot have multiple SET ACCESS METHOD subcommands
+DROP MATERIALIZED VIEW heapmv;
 DROP TABLE heaptable;
 -- No support for partitioned tables.
 CREATE TABLE am_partitioned(x INT, y INT)
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index 473fe8c28e..c52cf1cfcf 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -905,6 +905,16 @@ SELECT COUNT(*) FROM testschema.atable;		-- checks heap
  3
 (1 row)
 
+-- let's try moving a materialized view from one place to another
+CREATE MATERIALIZED VIEW testschema.amv AS SELECT * FROM testschema.atable;
+ALTER MATERIALIZED VIEW testschema.amv SET TABLESPACE regress_tblspace;
+REFRESH MATERIALIZED VIEW testschema.amv;
+SELECT COUNT(*) FROM testschema.amv;
+ count 
+---
+ 3
+(1 row)
+
 -- Will fail with bad path
 CREATE TABLESPACE regress_badspace LOCATION '/no/such/location';
 ERROR:  directory "/no/such/location" does not exist
@@ -939,18 +949,22 @@ RESET ROLE;
 ALTER TABLESPACE 

Re: XID formatting and SLRU refactorings

[Kyotaro Horiguchi]

At Fri, 18 Mar 2022 10:20:08 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> "(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
> point here is "%llu in format string accepts (long long)" so we should

Of course it's the typo of
 "%llu in format string accepts (*unsigned* long long)".

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Corruption during WAL replay

[Kyotaro Horiguchi]

At Wed, 16 Mar 2022 10:14:56 -0400, Robert Haas  wrote 
in 
> Hmm. I think the last two instances of "buffers" in this comment
> should actually say "blocks".

Ok. I replaced them with "blocks" and it looks nicer. Thanks!

> > I'll try that, if you are already working on it, please inform me. (It
> > may more than likely be too late..)
> 
> If you want to take a crack at that, I'd be delighted.

Finally, no two of from 10 to 14 doesn't accept the same patch.

As a cross-version check, I compared all combinations of the patches
for two adjacent versions and confirmed that no hunks are lost.

All versions pass check world.


The differences between each two adjacent versions are as follows.

master->14:

 A hunk fails due to the change in how to access rel->rd_smgr.

14->13:

  Several hunks fail due to simple context differences.

13->12:

 Many hunks fail due to the migration of delayChkpt from PGPROC to
 PGXACT and the context difference due to change of FSM trancation
 logic in RelationTruncate.

12->11:

 Several hunks fail due to the removal of volatile qalifier from
 pointers to PGPROC/PGXACT.

11-10:

 A hunk fails due to the context difference due to an additional
 member tempNamespaceId of PGPROC.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From c88858d3e5681005ba0396b7e7ebcde4322b3308 Mon Sep 17 00:00:00 2001
From: Robert Haas 
Date: Tue, 15 Mar 2022 12:29:14 -0400
Subject: [PATCH] Fix possible recovery trouble if TRUNCATE overlaps a
 checkpoint.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If TRUNCATE causes some buffers to be invalidated and thus the
checkpoint does not flush them, TRUNCATE must also ensure that the
corresponding files are truncated on disk. Otherwise, a replay
from the checkpoint might find that the buffers exist but have
the wrong contents, which may cause replay to fail.

Report by Teja Mupparti. Patch by Kyotaro Horiguchi, per a design
suggestion from Heikki Linnakangas, with some changes to the
comments by me. Review of this and a prior patch that approached
the issue differently by Heikki Linnakangas, Andres Freund, Álvaro
Herrera, Masahiko Sawada, and Tom Lane.

Back-patch to all supported versions.

Discussion: http://postgr.es/m/byapr06mb6373bf50b469ca393c614257ab...@byapr06mb6373.namprd06.prod.outlook.com
---
 src/backend/access/transam/multixact.c  |  6 ++--
 src/backend/access/transam/twophase.c   | 12 
 src/backend/access/transam/xact.c   |  5 ++--
 src/backend/access/transam/xlog.c   | 16 +--
 src/backend/access/transam/xloginsert.c |  2 +-
 src/backend/catalog/storage.c   | 29 ++-
 src/backend/storage/buffer/bufmgr.c |  6 ++--
 src/backend/storage/ipc/procarray.c | 26 -
 src/backend/storage/lmgr/proc.c |  4 +--
 src/include/storage/proc.h  | 37 -
 src/include/storage/procarray.h |  5 ++--
 11 files changed, 120 insertions(+), 28 deletions(-)

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 6a70d49738..9f65c600d0 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3088,8 +3088,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
 	 * crash/basebackup, even though the state of the data directory would
 	 * require it.
 	 */
-	Assert(!MyProc->delayChkpt);
-	MyProc->delayChkpt = true;
+	Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+	MyProc->delayChkpt |= DELAY_CHKPT_START;
 
 	/* WAL log truncation */
 	WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3115,7 +3115,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
 	/* Then offsets */
 	PerformOffsetsTruncation(oldestMulti, newOldestMulti);
 
-	MyProc->delayChkpt = false;
+	MyProc->delayChkpt &= ~DELAY_CHKPT_START;
 
 	END_CRIT_SECTION();
 	LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 874c8ed125..4dc8ccc12b 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -475,7 +475,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
 	}
 	proc->xid = xid;
 	Assert(proc->xmin == InvalidTransactionId);
-	proc->delayChkpt = false;
+	proc->delayChkpt = 0;
 	proc->statusFlags = 0;
 	proc->pid = 0;
 	proc->databaseId = databaseid;
@@ -1164,7 +1164,8 @@ EndPrepare(GlobalTransaction gxact)
 
 	START_CRIT_SECTION();
 
-	MyProc->delayChkpt = true;
+	Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+	MyProc->delayChkpt |= DELAY_CHKPT_START;
 
 	XLogBeginInsert();
 	for (record = records.head; record != NULL; record = record->next)
@@ -1207,7 +1208,7 @@ EndPrepare(GlobalTransaction gxact)
 	 * checkpoint starting after this will certainly see the gxact as a
 	 * candidate for fsyncing.
 	 */
-	MyProc->delayChkpt = false;
+	

Re: XID formatting and SLRU refactorings

[Kyotaro Horiguchi]

At Thu, 17 Mar 2022 19:25:00 +0300, Maxim Orlov  wrote in 
> Hi!
> 
> Here is the v20 patch. 0001 and 0002 are proposed into PG15 as
> discussed above.
> The whole set of patches is added into [1] to be committed into PG16.
> 
> In this version we've made a major revision related to printf/elog format
> compatible with localization
> as was proposed above.
> 
> We also think of adding 0003 patch related to 64 bit GUC's into this
> thread. Suppose it also may be delivered
> into PG15.
> 
> Aleksander Alekseev, we've done this major revision mentioned above and you
> are free to continue working on this patch set.
> 
> Reviews and proposals are very welcome!

(I'm afraid that this thread is not for the discussion of the patch?:)

> [1]
> https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

+/* printf/elog format compatible with 32 and 64 bit xid. */
+typedef unsigned long long XID_TYPE;
...
+errmsg_internal("found multixact %llu from before relminmxid %llu",
+(XID_TYPE) multi, (XID_TYPE) relminmxid)));

"(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
point here is "%llu in format string accepts (long long)" so we should
use literally (or bare) (long long) and the maybe-all precedents does
that.

And.. You looks like applying the change to some inappropriate places?

-  elog(DEBUG2, "deleted page from block %u has safexid %u",
- blkno, opaque->btpo_level);
+  elog(DEBUG2, "deleted page from block %u has safexid %llu",
+ blkno, (XID_TYPE) opaque->btpo_level);

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Out-of-tree certificate interferes ssltest

[Michael Paquier]

On Thu, Mar 17, 2022 at 02:28:49PM +0100, Daniel Gustafsson wrote:
> One small concern though. This hunk:
> 
> +my $default_ssl_connstr = "sslkey=invalid sslcert=invalid 
> sslrootcert=invalid sslcrl=invalid sslcrldir=invalid";
> +
>  $common_connstr =
> -  "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR 
> host=common-name.pg-ssltest.test";
> +  "$default_ssl_connstr user=ssltestuser dbname=trustdb 
> hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
>  
> ..together with the following changes along the lines of:
> 
> - "$common_connstr sslrootcert=invalid sslmode=require",
> + "$common_connstr sslmode=require",
> 
> ..is making it fairly hard to read the test and visualize what the connection
> string is and how the test should behave.  I don't have a better idea off the
> top of my head right now, but I think this is an area to revisit and improve
> on.

I agree that this makes this set of three tests harder to follow, as
we expect a root cert to *not* be set locally.  Keeping the behavior
documented in each individual string would be better, even if that
duplicates more the keys in those final strings.

Another thing that Horiguchi-san has pointed out upthread (?) is 003,
where it is also possible to trigger failures once the environment is
hijacked.  The attached allows the full test suite to pass without
issues on my side.
--
Michael
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..605e405de3 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -138,8 +138,13 @@ note "running client tests";
 
 switch_server_cert($node, 'server-cn-only');
 
+# Set of default settings for SSL parameters in connection string.  This
+# makes the tests protected against any defaults the environment may have
+# in ~/.postgresql/.
+my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid sslcrl=invalid sslcrldir=invalid";
+
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
 
 # The server should not accept non-SSL connections.
 $node->connect_fails(
@@ -216,9 +221,10 @@ $node->connect_fails(
 	"CRL belonging to a different CA",
 	expected_stderr => qr/SSL error: certificate verify failed/);
 
-# The same for CRL directory
+# The same for CRL directory.  sslcrl='' is added here to override the
+# invalid default, so as this does not interfere with this case.
 $node->connect_fails(
-	"$common_connstr sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
+	"$common_connstr sslcrl='' sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrldir=ssl/client-crldir",
 	"directory CRL belonging to a different CA",
 	expected_stderr => qr/SSL error: certificate verify failed/);
 
@@ -235,7 +241,7 @@ $node->connect_ok(
 # Check that connecting with verify-full fails, when the hostname doesn't
 # match the hostname in the server's certificate.
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
 
 $node->connect_ok("$common_connstr sslmode=require host=wronghost.test",
 	"mismatch between host name and server certificate sslmode=require");
@@ -253,7 +259,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-multiple-alt-names');
 
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
 $node->connect_ok(
 	"$common_connstr host=dns1.alt-name.pg-ssltest.test",
@@ -282,7 +288,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-single-alt-name');
 
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
 $node->connect_ok(
 	"$common_connstr host=single.alt-name.pg-ssltest.test",
@@ -306,7 +312,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-cn-and-alt-names');
 
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
 $node->connect_ok("$common_connstr host=dns1.alt-name.pg-ssltest.test",
 	"certificate with both a CN and SANs 1");
@@ -323,7 

Re: Declare PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY for aarch64

[Thomas Munro]

On Thu, Mar 17, 2022 at 1:32 AM Yura Sokolov  wrote:
> So I believe it is safe to define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
> for aarch64

Agreed, and pushed.  There was another thread that stalled, so I added
a reference and a reviewer from that to your commit message.

This should probably also be set for RISCV64.

Re: Support logical replication of DDLs

[Japin Li]

On Fri, 18 Mar 2022 at 08:22, Japin Li  wrote:
> On Fri, 18 Mar 2022 at 00:22, Zheng Li  wrote:
>> Hello Japin,
>>>The new patch change the behavior of publication, when we drop a table
>>>on publisher, the table also be dropped on subscriber, and this made the
>>>regression testa failed, since the regression test will try to drop the
>>>table on subscriber.
>>
>>>IMO, we can disable the DDLs replication by default, which makes the
>>>regression test happy.  Any thoughts?
>>
>> Good catch, I forgot to run the whole TAP test suite. I think for now
>> we can simply disable DDL replication for the failing tests when
>> creating publication: $node_publisher->safe_psql('postgres',
>> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
>> I'll fix it in the next version of patch. I'll also work on breaking
>> down the patch into smaller pieces for ease of review.
>>
>
> Oh, it doesn't work.
>
>   postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
>   ERROR:  conflicting or redundant options
>
>
> Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.
>
> + if (*ddl_level_given)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +  errmsg("conflicting or 
> redundant options")));
> +
> + /*
> +  * If publish option was given only the explicitly 
> listed actions
> +  * should be published.
> +  */
> + pubactions->pubddl_database = false;
> + pubactions->pubddl_table = false;
> +
> + *ddl_level_given = true;


Oh, Sorry, I misunderstand it, it just like you forget initialize
*ddl_level_given to false when enter parse_publication_options().


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Tab completion for SET TimeZone

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?=  writes:
>> I just noticed I left out the = in the match check, here's an updated
>> patch that fixes that.
>
> Hmm  is that actually going to be useful in that form?
> Most time zone names contain slashes and will therefore require
> single-quoting.  I think you might need pushups comparable to
> COMPLETE_WITH_ENUM_VALUE.

With readline (which is what I tested on) the completion works with or
without a single quote, but the user has to supply the quote themselves
for non-identifier-syntax timezone names.

I wasn't aware of the difference in behaviour with libedit, but now that
I've tested I agree that quoting things even when not strictly needed is
better.

This does however have the unfortunate side effect that on readline it
will suggest DEFAULT even after a single quote, which is not valid.

> Also, personally, I'd rather not smash the names to lower case.
> I think that's a significant decrement of readability.

That was mainly for convenience of not having to capitalise the place
names when typing (since they are accepted case insensitively).  A
compromise would be to lower-case it in the WHERE, but not in the
SELECT, as in the attached v3 patch.

I've tested this version on Debian stable with both readline 8.1-1 and
libedit 3.1-20191231-2.

- ilmari

>From 07ed215e983fe4fda10cf92ddd12991f76e1410d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 16 Mar 2022 12:52:21 +
Subject: [PATCH v3] =?UTF-8?q?Add=20tab=20completion=20for=20SET=20TimeZon?=
 =?UTF-8?q?e=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Because the value (usually) needs single-quoting, and libedit includes
the leading quote in the text passed to the tab completion, but
readline doesn't, handle it simlarly to what we do for enum values.

This does however mean that under readline it'll include DEFAULT even
after an opening single quote, which is not valid.

Match then names case-insensitively for convenience, but return them
in the original case for legibility.
---
 src/bin/psql/tab-complete.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17172827a9..bb2e6f47a7 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -402,6 +402,19 @@ do { \
 	matches = rl_completion_matches(text, complete_from_schema_query); \
 } while (0)
 
+#define COMPLETE_WITH_TIMEZONE_NAME()			\
+do { \
+	static const char *const list[] = { "DEFAULT", NULL }; \
+	if (text[0] == '\'' || \
+		start == 0 || rl_line_buffer[start - 1] != '\'') \
+		completion_charp = Query_for_list_of_timezone_names_quoted; \
+	else \
+		completion_charp = Query_for_list_of_timezone_names_unquoted; \
+	completion_charpp = list;			  \
+	completion_verbatim = true; \
+	matches = rl_completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
 	set_completion_reference(function); \
@@ -1105,6 +1118,18 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"
 
+#define Query_for_list_of_timezone_names_unquoted \
+" SELECT name "\
+"   FROM pg_catalog.pg_timezone_names() "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
+#define Query_for_list_of_timezone_names_quoted \
+" SELECT name FROM ("\
+"   SELECT pg_catalog.quote_literal(name) AS name "\
+" FROM pg_catalog.pg_timezone_names() "\
+"  ) ss "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4171,6 +4196,8 @@ psql_completion(const char *text, int start, int end)
 	 " AND nspname NOT LIKE E'pg_temp%%'",
 	 "DEFAULT");
 		}
+		else if (TailMatches("TimeZone", "TO|="))
+			COMPLETE_WITH_TIMEZONE_NAME();
 		else
 		{
 			/* generic, type based, GUC support */
-- 
2.30.2

Re: Support logical replication of DDLs

[Japin Li]

On Fri, 18 Mar 2022 at 08:18, Zheng Li  wrote:
> Hello,
>
> Attached please find the broken down patch set. Also fixed the failing
> TAP tests Japin reported.
>

Thanks for updating the patchset, I will try it later.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Support logical replication of DDLs

[Japin Li]

On Fri, 18 Mar 2022 at 00:22, Zheng Li  wrote:
> Hello Japin,
>>The new patch change the behavior of publication, when we drop a table
>>on publisher, the table also be dropped on subscriber, and this made the
>>regression testa failed, since the regression test will try to drop the
>>table on subscriber.
>
>>IMO, we can disable the DDLs replication by default, which makes the
>>regression test happy.  Any thoughts?
>
> Good catch, I forgot to run the whole TAP test suite. I think for now
> we can simply disable DDL replication for the failing tests when
> creating publication: $node_publisher->safe_psql('postgres',
> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
> I'll fix it in the next version of patch. I'll also work on breaking
> down the patch into smaller pieces for ease of review.
>

Oh, it doesn't work.

postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
ERROR:  conflicting or redundant options


Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.

+   if (*ddl_level_given)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting or 
redundant options")));
+
+   /*
+* If publish option was given only the explicitly 
listed actions
+* should be published.
+*/
+   pubactions->pubddl_database = false;
+   pubactions->pubddl_table = false;
+
+   *ddl_level_given = true;


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Skipping logical replication transactions on subscriber side

[Masahiko Sawada]

On Thu, Mar 17, 2022 at 3:03 PM Amit Kapila  wrote:
>
> On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada  wrote:
> >
> > I've attached an updated version patch.
> >
>
> The patch LGTM. I have made minor changes in comments and docs in the
> attached patch. Kindly let me know what you think of the attached?

Thank you for updating the patch. It looks good to me.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: ICU for global collation

[Andres Freund]

Hi,

On 2022-03-17 14:14:52 +0100, Peter Eisentraut wrote:
> On 17.03.22 13:01, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
> > Thank you to all the developers.
> > I found that the description of the pg_database.daticulocale column was not 
> > written in the documentation.
> > The attached small patch adds a description of the daticulocale column to 
> > catalogs.sgml.
> 
> committed, thanks

Wee! That's a long time weakness addressed...


Just saw a weird failure after rebasing my meson branch ontop of this. Tests
passed on debian, suse, centos 8 stream, fedora rawhide (failed due to an
independent reason), but not on centos 7.


all runs: https://cirrus-ci.com/build/5190538184884224
centos 7: https://cirrus-ci.com/task/4786632883699712?logs=tests_world#L204
centos 7 failure: 
https://api.cirrus-ci.com/v1/artifact/task/4786632883699712/log/build/testrun/icu/t/010_database/log/regress_log_010_database

not ok 1 - sort by database default locale
#   Failed test 'sort by database default locale'
#   at /tmp/cirrus-ci-build/src/test/icu/t/010_database.pl line 28.
#  got: 'a
# A
# b
# B'
# expected: 'A
# a
# B
# b'
ok 2 - sort by explicit collation standard
not ok 3 - sort by explicit collation upper first
#   Failed test 'sort by explicit collation upper first'
#   at /tmp/cirrus-ci-build/src/test/icu/t/010_database.pl line 42.
#  got: 'a
# A
# b
# B'
# expected: 'A
# a
# B
# b'
ok 4 - ICU locale must be specified for ICU provider: exit code not 0
ok 5 - ICU locale must be specified for ICU provider: error message
1..5

This is a run building with meson. But I've now triggered builds with autoconf
on centos 7 as well and that also failed. See
https://cirrus-ci.com/task/6194007767252992?logs=test_world#L378

So it looks like older ICU versions don't work?

Greetings,

Andres Freund

PS: I had not yet passed with_icu in the initdb tests for meson, that's why
there's two failures with autoconf but only one with meson.

Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

[Nathan Bossart]

I think this one requires some more work, and it needn't be a priority for
v15, so I've adjusted the commitfest entry to v16 and moved it to the next
commitfest.

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

Re: support for CREATE MODULE

[Nathan Bossart]

On Thu, Mar 17, 2022 at 04:26:31PM -0700, Swaha Miller wrote:
> On Thu, Mar 17, 2022 at 4:16 PM Nathan Bossart 
> wrote:
> 
>> It seems unlikely that this will be committed for v15.  Swaha, should the
>> commitfest entry be adjusted to v16 and moved to the next commitfest?
>>
>>
> Yes please, thank you Nathan

Done!

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

Re: support for CREATE MODULE

[Swaha Miller]

On Thu, Mar 17, 2022 at 4:16 PM Nathan Bossart 
wrote:

> It seems unlikely that this will be committed for v15.  Swaha, should the
> commitfest entry be adjusted to v16 and moved to the next commitfest?
>
>
Yes please, thank you Nathan

Re: Patch to avoid orphaned dependencies

[Nathan Bossart]

Bertand, do you think this has any chance of making it into v15?  If not,
are you alright with adjusting the commitfest entry to v16 and moving it to
the next commitfest?

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

Re: support for CREATE MODULE

[Nathan Bossart]

It seems unlikely that this will be committed for v15.  Swaha, should the
commitfest entry be adjusted to v16 and moved to the next commitfest?

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

Re: Pre-allocating WAL files

[Nathan Bossart]

It seems unlikely that this will be committed for v15, so I've adjusted the
commitfest entry to v16 and moved it to the next commitfest.

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

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

[Nathan Bossart]

It seems unlikely that anything discussed in this thread will be committed
for v15, so I've adjusted the commitfest entry to v16 and moved it to the
next commitfest.

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

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Tom Lane]

Jacob Champion  writes:
> On Fri, 2022-03-04 at 10:45 +0900, Michael Paquier wrote:
>> At the end of the day, Port is an interface used for the communication
>> between the postmaster with the frontends, so I'd like to say that it
>> is correct to not apply this concept to parallel workers because they
>> are not designed to contact any frontend-side things.

> Coming back to this late, sorry. I'm not quite sure where to move with
> this. I'm considering copying pieces of Port over just so we can see
> what it looks like in practice?

> Personally I think it makes sense for the parallel workers to have the
> authn information for the client -- in fact there's a lot of
> information that it seems shouldn't be hidden from them -- but there
> are other pieces, like the socket handle, that are clearly not useful.

Yeah.  It seems to me that putting the auth info into struct Port was
a fairly random thing to do in the first place, and we are now dealing
with the fallout of that.

I think what we ought to do here is separate out the data that we think
parallel workers need access to.  It does not seem wise to say "workers
can access fields A,B,C of MyPort but not fields X,Y,Z".  I do not have
a concrete proposal for fixing it though.

regards, tom lane

Re: ExecTypeSetColNames is fundamentally broken

[Tom Lane]

Aleksander Alekseev  writes:
> I understand the concern expressed by Robert in respect of backward
> compatibility. From the user's perspective, personally I would prefer
> a fixed bug over backward compatibility. Especially if we consider the
> fact that the current behaviour of cases like `select row_to_json(i)
> from int8_tbl i(x,y)` is not necessarily the correct one, at least it
> doesn't look right to me.

It's debatable anyway.  I'd be less worried about changing this behavior
if we didn't have to back-patch it ... but I see no good alternative.

> So unless anyone has strong objections against the proposed fix or can
> propose a better patch, I would suggest merging it.

Done now.

regards, tom lane

Re: [PATCH] Expose port->authn_id to extensions and triggers

[Jacob Champion]

On Fri, 2022-03-04 at 10:45 +0900, Michael Paquier wrote:
> At the end of the day, Port is an interface used for the communication
> between the postmaster with the frontends, so I'd like to say that it
> is correct to not apply this concept to parallel workers because they
> are not designed to contact any frontend-side things.

Coming back to this late, sorry. I'm not quite sure where to move with
this. I'm considering copying pieces of Port over just so we can see
what it looks like in practice?

Personally I think it makes sense for the parallel workers to have the
authn information for the client -- in fact there's a lot of
information that it seems shouldn't be hidden from them -- but there
are other pieces, like the socket handle, that are clearly not useful.

Thanks,
--Jacob

Re: [PATCH] Accept IP addresses in server certificate SANs

[Jacob Champion]

On Wed, 2022-03-16 at 23:49 +, Jacob Champion wrote:
> Thank you for the explanation -- the misunderstanding was all on my
> end. I thought you were asking me to move the check_cn assignment
> instead of copying it to the end. I agree that your suggestion is much
> clearer, and I'll make that change tomorrow.

Done in v8. Thanks again for your suggestions (and for your
perseverance when I didn't get it)!

--Jacob
diff --git a/src/interfaces/libpq/fe-secure-openssl.c 
b/src/interfaces/libpq/fe-secure-openssl.c
index c6a80d30c2..06b166b7fd 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -650,7 +650,14 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn 
*conn,
}
 
if (rc != 0)
+   {
+   /*
+* Either we hit an error or a match, and 
either way we should
+* not fall back to the CN.
+*/
+   check_cn = false;
break;
+   }
}
sk_GENERAL_NAME_pop_free(peer_san, GENERAL_NAME_free);
}
@@ -663,7 +670,7 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn 
*conn,
 * dNSName is present, the CN must be ignored. We break this rule if 
host is
 * an IP address; see the comment above.)
 */
-   if ((rc == 0) && check_cn)
+   if (check_cn)
{
X509_NAME  *subject_name;
 
From 84a107da33b7d901cb318f8c2fd140644fbc5100 Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Wed, 24 Nov 2021 14:46:11 -0800
Subject: [PATCH v8 1/3] Move inet_net_pton() to src/port

This will be helpful for IP address verification in libpq.
---
 src/backend/utils/adt/Makefile  |  1 -
 src/include/port.h  |  3 +++
 src/include/utils/builtins.h|  4 
 src/port/Makefile   |  1 +
 src/{backend/utils/adt => port}/inet_net_pton.c | 16 +++-
 src/tools/msvc/Mkvcbuild.pm |  2 +-
 6 files changed, 20 insertions(+), 7 deletions(-)
 rename src/{backend/utils/adt => port}/inet_net_pton.c (96%)

diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 41b486bcef..d173d52157 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -43,7 +43,6 @@ OBJS = \
 	geo_selfuncs.o \
 	geo_spgist.o \
 	inet_cidr_ntop.o \
-	inet_net_pton.o \
 	int.o \
 	int8.o \
 	json.o \
diff --git a/src/include/port.h b/src/include/port.h
index 3d103a2b31..2852e5b58b 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -515,6 +515,9 @@ extern int	pg_codepage_to_encoding(UINT cp);
 extern char *pg_inet_net_ntop(int af, const void *src, int bits,
 			  char *dst, size_t size);
 
+/* port/inet_net_pton.c */
+extern int	pg_inet_net_pton(int af, const char *src, void *dst, size_t size);
+
 /* port/pg_strong_random.c */
 extern void pg_strong_random_init(void);
 extern bool pg_strong_random(void *buf, size_t len);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 666e545496..67b9326dc8 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -93,10 +93,6 @@ extern int	xidLogicalComparator(const void *arg1, const void *arg2);
 extern char *pg_inet_cidr_ntop(int af, const void *src, int bits,
 			   char *dst, size_t size);
 
-/* inet_net_pton.c */
-extern int	pg_inet_net_pton(int af, const char *src,
-			 void *dst, size_t size);
-
 /* network.c */
 extern double convert_network_to_scalar(Datum value, Oid typid, bool *failure);
 extern Datum network_scan_first(Datum in);
diff --git a/src/port/Makefile b/src/port/Makefile
index bfe1feb0d4..c3ae7b3d5c 100644
--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -43,6 +43,7 @@ OBJS = \
 	bsearch_arg.o \
 	chklocale.o \
 	inet_net_ntop.o \
+	inet_net_pton.o \
 	noblock.o \
 	path.o \
 	pg_bitutils.o \
diff --git a/src/backend/utils/adt/inet_net_pton.c b/src/port/inet_net_pton.c
similarity index 96%
rename from src/backend/utils/adt/inet_net_pton.c
rename to src/port/inet_net_pton.c
index d3221a1313..bae50ba67e 100644
--- a/src/backend/utils/adt/inet_net_pton.c
+++ b/src/port/inet_net_pton.c
@@ -14,14 +14,18 @@
  * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT
  * OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  *
- *	  src/backend/utils/adt/inet_net_pton.c
+ *	  src/port/inet_net_pton.c
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
 static const char rcsid[] = "Id: inet_net_pton.c,v 1.4.2.3 2004/03/17 00:40:11 marka Exp $";
 #endif
 
+#ifndef FRONTEND
 #include "postgres.h"
+#else
+#include "postgres_fe.h"
+#endif
 
 #include 
 #include 
@@ -29,9 +33,19 @@ static const char rcsid[] = "Id: inet_net_pton.c,v 1.4.2.3 2004/03/17 00:40:11 m
 

Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

On 2022-03-16 18:50:23 -0400, Stephen Frost wrote:
> First, let's be clear- we aren't actually talking about custom or
> pluggable authentication here, at least when it comes to PG as a
> project.  For it to actually be pluggable, it needs to be supported on
> both the client side and the server side, not just the server side.
> 
> That this keeps getting swept under the carpet makes me feel like this
> isn't actually an argument about the best way to move the PG project
> forward but rather has another aim.

This is insulting and unjustified. IMO completely inappropriate for the list /
community. I've also brought this up privately, but I thought it important to
state so publically.

Re: WIP: WAL prefetch (another approach)

[Thomas Munro]

On Mon, Mar 14, 2022 at 8:17 PM Julien Rouhaud  wrote:
> Great!  I'm happy with 0001 and I think it's good to go!

I'll push 0001 today to let the build farm chew on it for a few days
before moving to 0002.

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

[Nathan Bossart]

On Thu, Mar 17, 2022 at 06:48:43PM +0530, Bharath Rupireddy wrote:
> Personally, I tend to agree with v4-0001 (option (4)) or v4-0002
> (option (3)) than v4-0003 (option (1)) as it adds more unreadability
> with most of the code duplicated creating more diff with previous
> versions and maintainability problems. Having said that, I will leave
> it to the committer to decide on that.

I don't think v4-0003/option 1 needs to be unreadable.  Perhaps we can use
a StringInfo to build the message.  That might be a net improvement in
readability anyway.

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

Re: proposal: enhancing plpgsql debug API - returns text value of variable content

[Pavel Stehule]

Hi

čt 17. 3. 2022 v 20:58 odesílatel Greg Stark  napsal:

> It looks like this is -- like a lot of plpgsql patches -- having
> difficulty catching the attention of reviewers and committers.
> Aleksander asked for a test and Pavel put quite a bit of work into
> adding a good test case. I actually like that there's a test because
> it shows the API can be used effectively.
>
> From my quick skim of this code it does indeed look like it's ready to
> commit. It's mostly pretty mechanical code to expose a couple fields
> so that a debugger can see them.
>
> Pavel, are you planning to add a debugger to contrib using this? The
> test example code looks like it would already be kind of useful even
> in this form.
>

I had a plan to use the new API in plpgsql_check
https://github.com/okbob/plpgsql_check for integrated tracer

There is the pldebugger https://github.com/EnterpriseDB/pldebugger and I
think this extension can use this API well too. You can see - the PL
debugger has about 6000 lines, and more, there are some extensions for EDB.
So, unfortunately, It doesn't looks like good candidate for contrib.
Writing new debugger from zero doesn't looks like effective work. I am open
any discussion about it. Maybe some form of more sophisticated tracer can
be invited, but I think so better is enhancing widely used pldebugger
extension.

Regards

Pavel

Re: refactoring basebackup.c (zstd workers)

[Robert Haas]

Thanks for the review!

I'll address most of these comments later, but quickly for right now...

On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby  wrote:
> It'd be great if this were re-usable for wal_compression, which I hope in 
> pg16 will
> support at least level=N.  And eventually pg_dump.  But those clients 
> shouldn't
> accept a client/server prefix.  Maybe the way to handle that is for those 
> tools
> to check locationres and reject it if it was specified.
> [...]
> This is confusingly similar to src/include/access/xlog.h:WalCompression.
> I think someone else mentioned this before ?

A couple of people before me have had delusions of grandeur in this
area. We have the WalCompression enum, which has values of the form
COMPRESSION_*, instead of WAL_COMPRESSION_*, as if the WAL were going
to be the only thing that ever got compressed. And pg_dump.h also has
a CompressionAlgorithm enum, with values like COMPR_ALG_*, which isn't
great naming either. Clearly there's some cleanup needed here: if we
can use the same enum for multiple systems, then it can have a name
implying that it's the only game in town, but otherwise both the enum
name and the corresponding value need to use a suitable prefix. I
think that's a job for another patch, probably post-v15. For now I
plan to do the right thing with the new names I'm adding, and leave
the existing names alone. That can be changed in the future, if and
when it seems sensible.

As I said elsewhere, I think the WAL compression stuff is badly
designed and should probably be rewritten completely, maybe to reuse
the bbstreamer stuff. In that case, WalCompressionMethod would
probably go away entirely, making the naming confusion moot, and
picking up zstd and lz4 compression support for free. If that doesn't
happen, we can probably find some way to at least make them share an
enum, but I think that's too hairy to try to clean up right now with
feature freeze pending.

> The server crashes if I send an unknown option - you should hit that in the
> regression tests.
>
> $ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp 
> --no-sync --no-manifest --compress=server-lz4:a |wc -c
> TRAP: FailedAssertion("pointer != NULL", File: 
> "../../../../src/include/utils/memutils.h", Line: 123, PID: 8627)
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(ExceptionalCondition+0xa0)[0x560b45d7b64b]
> postgres: walsender pryzbyj [local] BASE_BACKUP(pfree+0x5d)[0x560b45dad1ea]
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(parse_bc_specification+0x154)[0x560b45dc5d4f]
> postgres: walsender pryzbyj [local] BASE_BACKUP(+0x43d56c)[0x560b45bc556c]
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(SendBaseBackup+0x2d)[0x560b45bc85ca]
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(exec_replication_command+0x3a2)[0x560b45bdddb2]
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(PostgresMain+0x6b2)[0x560b45c39131]
> postgres: walsender pryzbyj [local] BASE_BACKUP(+0x40530e)[0x560b45b8d30e]
> postgres: walsender pryzbyj [local] BASE_BACKUP(+0x408572)[0x560b45b90572]
> postgres: walsender pryzbyj [local] BASE_BACKUP(+0x4087b9)[0x560b45b907b9]
> postgres: walsender pryzbyj [local] 
> BASE_BACKUP(PostmasterMain+0x1135)[0x560b45b91d9b]
> postgres: walsender pryzbyj [local] BASE_BACKUP(main+0x229)[0x560b45ad0f78]

That's odd - I thought I had tested that case. Will double-check.

> This is interpreted like client-gzip-1; should multiple specifications of
> compress be prohibited ?
>
> | src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp 
> --no-sync --no-manifest --compress=server-lz4 --compress=1

They're not now and haven't been in the past. I think the last one
should just win (as it apparently does, here). We do that in some
places and throw an error in others and I'm not sure if we have a 100%
consistent rule for it, but flipping one location between one behavior
and the other isn't going to make things more consistent overall.

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

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2022-03-17 14:03:31 -0400, Stephen Frost wrote:
> > * Andres Freund (and...@anarazel.de) wrote:
> > > On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > > > Looking at the existing authentication methods
> > > > 
> > > > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > > > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> > > > 
> > > > how many of these could have been implemented using a plugin mechanism 
> > > > that
> > > > was designed before the new method was considered?  Probably not many.
> > > 
> > > trust, reject, md5, password, ident, peer, pam, ldap, radius look 
> > > trivially
> > > possible. I think sspi is doable as well, but I don't know it well enough 
> > > to
> > > be confident. gss without transport encryption could have as well, I
> > > think. Even scram-sha-256 looks possible, although that'd have been a 
> > > good bit
> > > harder.  Where do you see the problems?
> > 
> > I agree with Peter and don't feel like the above actually answered the
> > question in any fashion.  The question wasn't "which auth methods could
> > be implemented using these new set of hooks?" but rather- which could
> > have been implemented using a plugin mechanism that was designed
> > *before* the new method was considered?  The answer to that is
> > 'basically none'.
> 
> I fail to see how you come to that conclusion.

...

> > There even existed a plugin mechanism (PAM) before many of them, and they
> > weren't able implemented using it.
> 
> That's nonsensical. PAM is a platform specific API to start with - so it
> doesn't make sense to implement additional postgres authentication mechanism
> with it. It also wasn't added to postgres as a base mechanism for extensible
> authentication. And it's fundamentally restricted in the kind of secret
> exchanges that can be implemented with it.

Exactly because of this.  The folks who came up with PAM didn't forsee
the direction that authentication methods were going to go in and I
don't see any particular reason why we're smarter than they were.

> > That's entirely the point- while this might be an interesting way to
> > redesign what we have now, should we feel that's useful to do (I don't and
> > think it would be an actively bad thing for the project to try and do...)
> > there's no reason to believe that it'll actually be useful for the *next*
> > auth method that comes along.
> 
> What concrete limitation do you see in the API? It basically gives you the
> same powers as writing something in auth.c directly. And due to AUTH_REQ_SASL*
> we have fairly extensible ways of exchanging authentication data.

I'm not one to predict what the next authentication method to come down
the road is but I doubt that an API we come up with today will actually
work to perfectly implement it.  GSSAPI was supposed to be an extensible
authentication method too.

Also- how is this going to work client-side in libpq?

> > > Only stuff tying into transport encryption is clearly not doable via the
> > > proposed API, but that's hardly the fault of an authentication API?
> > 
> > This is absolutely the wrong way to look at it.  The authentication
> > methods that are coming along today are designed to tie into the
> > transport encryption because that's *needed* to avoid MITM attacks.  I'd
> > much rather we generally avoid including ones that don't support that
> > and we certainly shouldn't be trying to build a generic system which
> > explicitly doesn't support it.
> 
> So you're saying that any authentication API needs to come together with a
> runtime extendable secure transport mechanism? That feels like raising the bar
> ridiculously into the sky. If we want to add more transport mechanisms - and
> I'm not sure we do - that's a very sizable project on its own. And
> independent.

No, that's not what I'm saying.  Authentication mechanisms should have a
way to tie themselves to the secure transport layer is what I was
getting at, which you seemed to be saying wasn't possible with this API.

However, it's certainly a relevant point that something like encrypted
GSSAPI still wouldn't be able to be implemented with this.  I don't know
that it's required for a new auth method to have that, but not being
able to do it with the proposed API is another point against this
approach.  That is, even the existing methods that we've got today
wouldn't all be able to work with this.

> Note that you *can* combine existing secure transport mechanisms with the
> proposed API. You can use Port->{ssl,gss,peer_cn,...} to query information and
> do further openssl etc calls.

If it's possible then that's good, though I do have concerns about how
this plays into support for different transport layers or even libraries
once we get support for an alternative to openssl.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Robert Haas]

On Wed, Mar 16, 2022 at 12:53 AM Dilip Kumar  wrote:
> Thanks Ashutosh and Robert.  Other pacthes cleanly applied on this
> patch still generated a new version so that we can find all patches
> together.  There are no other changes.

I committed my v3 of my refactoring patch, here 0001.

I'm working over the comments in the rest of the patch series and will
post an updated version when I get done. I think I will likely merge
all the remaining patches together just to make it simpler to manage;
we can split things out again if we need to do that.

One question that occurred to me when looking this over is whether, or
why, it's safe against concurrent smgr invalidations. It seems to me
that every loop in the new CREATE DATABASE code needs to
CHECK_FOR_INTERRUPTS() -- some do already -- and when they do that, I
think we might receive an invalidation message that causes us to
smgrclose() some or all of the things where we previously did
smgropen(). I don't quite see why that can't cause problems here. I
tried running the src/bin/scripts regression tests with
debug_discard_caches=1 and none of the tests failed, so there may very
well be a reason why this is actually totally fine, but I don't know
what it is. On the other hand, it may be that things went horribly
wrong and the tests are just smart enough to catch it, or maybe
there's a problematic scenario which those tests just don't hit. I
don't know. Thoughts?

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

Re: Optimize external TOAST storage

[Nathan Bossart]

On Thu, Mar 17, 2022 at 01:04:17PM -0400, Robert Haas wrote:
> Right, so perhaps the ultimate thing here would be a more fine-grained
> knob than SET STORAGE EXTERNAL -- something that allows you to specify
> that you want to compress only when it really helps. While some people
> might find that useful, I think the current patch is less ambitious,
> and I think that's OK. It just wants to save something in the cases
> where it's basically free. Unfortunately we've learned that it's never
> *entirely* free because making the last TOAST chunk longer can always
> cost you something, even if it gets longer by only 1 byte. But for
> larger values it's hard for that to be significant.

I guess I think we should be slightly more ambitious.  One idea could be to
create a default_toast_compression_ratio GUC with a default of 0.95.  This
means that, by default, a compressed attribute must be 0.95x or less of the
size of the uncompressed attribute to be stored compressed.  Like
default_toast_compression, this could also be overridden at the column
level with something like

ALTER TABLE mytbl ALTER mycol SET COMPRESSION lz4 RATIO 0.9;

If the current "basically free" patch is intended for v15, then maybe all
this extra configurability stuff could wait for a bit, especially if we can
demonstrate a decent read performance boost with a more conservative
setting.  However, I don't see anything terribly complicated about the
proposed configurability changes (assuming my proposal makes some amount of
sense to you and others).

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

Re: Proposal for internal Numeric to Uint64 conversion function.

[Tom Lane]

Greg Stark  writes:
> On Fri, 11 Mar 2022 at 15:17, Tom Lane  wrote:
>> The patch as-presented isn't very compelling for
>> lack of callers of the new function

> Tom, are you saying you think we're not interested in just adding this
> function unless it's part of this refactoring?

Pretty much, yeah.  I'm way more interested in cleaning up the code
we have than in making things prettier for hypothetical future
call sites.  In particular, the problem with writing an API in a
vacuum is that you have little evidence that it's actually useful
as given (e.g., did you handle error cases in a useful way).  If we
create a numeric_to_int64 that is actually used right away by some
existing callers, then we've got some evidence that we did it right;
and then introducing a parallel numeric_to_uint64 is less of a leap
of faith.

regards, tom lane

Re: proposal: enhancing plpgsql debug API - returns text value of variable content

[Greg Stark]

It looks like this is -- like a lot of plpgsql patches -- having
difficulty catching the attention of reviewers and committers.
Aleksander asked for a test and Pavel put quite a bit of work into
adding a good test case. I actually like that there's a test because
it shows the API can be used effectively.

>From my quick skim of this code it does indeed look like it's ready to
commit. It's mostly pretty mechanical code to expose a couple fields
so that a debugger can see them.

Pavel, are you planning to add a debugger to contrib using this? The
test example code looks like it would already be kind of useful even
in this form.

Re: Proposal for internal Numeric to Uint64 conversion function.

[Greg Stark]

On Fri, 11 Mar 2022 at 15:17, Tom Lane  wrote:
>
> Amul Sul  writes:

> >  Yeah, that's true, I am too not sure if we really need to refactor
> >  all those; If we want, I can give it a try.
>
> The patch as-presented isn't very compelling for
> lack of callers of the new function

Tom, are you saying you think we're not interested in just adding this
function unless it's part of this refactoring?

Amul, do you think if we did numeric_to_int64/numeric_to_uint64 as a
refactored API and a second patch that made numeric_pg_lsn and other
consumers use it it would clean up the code significantly?

-- 
greg

Re: refactoring basebackup.c (zstd workers)

[Justin Pryzby]

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9178c779ba..00c593f1af 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2731,14 +2731,24 @@ The commands accepted in replication mode are:
+
+  For gzip the compression level should be an

gzip comma

+++ b/src/backend/replication/basebackup.c
@@ -18,6 +18,7 @@
 
 #include "access/xlog_internal.h"  /* for pg_start/stop_backup */
 #include "common/file_perm.h"
+#include "common/backup_compression.h"

alphabetical

-errmsg("unrecognized 
compression algorithm: \"%s\"",
+errmsg("unrecognized 
compression algorithm \"%s\"",

Most other places seem to say "compression method".  So I'd suggest to change
that here, and in doc/src/sgml/ref/pg_basebackup.sgml.

-   if (o_compression_level && !o_compression)
+   if (o_compression_detail && !o_compression)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
 errmsg("compression level requires 
compression")));

s/level/detail/

 /*
+ * Basic parsing of a value specified for -Z/--compress.
+ *
+ * We're not concerned here with understanding exactly what behavior the
+ * user wants, but we do need to know whether the user is requesting client
+ * or server side compression or leaving it unspecified, and we need to
+ * separate the name of the compression algorithm from the detail string.
+ *
+ * For instance, if the user writes --compress client-lz4:6, we want to
+ * separate that into (a) client-side compression, (b) algorithm "lz4",
+ * and (c) detail "6". Note, however, that all the client/server prefix is
+ * optional, and so is the detail. The algorithm name is required, unless
+ * the whole string is an integer, in which case we assume "gzip" as the
+ * algorithm and use the integer as the detail.
..
  */
 static void
+parse_compress_options(char *option, char **algorithm, char **detail,
+  CompressionLocation *locationres)

It'd be great if this were re-usable for wal_compression, which I hope in pg16 
will
support at least level=N.  And eventually pg_dump.  But those clients shouldn't
accept a client/server prefix.  Maybe the way to handle that is for those tools
to check locationres and reject it if it was specified.

+ * We're not concerned with validation at this stage, so if the user writes
+ * --compress client-turkey:sandwhich, the requested algorithm is "turkey"
+ * and the detail string is "sandwhich". We'll sort out whether that's legal

sp: sandwich

+   WalCompressionMethodwal_compress_method;

This is confusingly similar to src/include/access/xlog.h:WalCompression.
I think someone else mentioned this before ?

+ * A compression specification specifies the parameters that should be used
+ * when * performing compression with a specific algorithm. The simplest

star

+/*
+ * Get the human-readable name corresponding to a particular compression
+ * algorithm.
+ */
+char *
+get_bc_algorithm_name(bc_algorithm algorithm)

should be const ?

+   /* As a special case, the specification can be a bare integer. */
+   bare_level = strtol(specification, _level_endp, 10);

Should this call expect_integer_value()?
See below.

+   result->parse_error =
+   pstrdup("found empty string where a compression 
option was expected");

Needs to be localized with _() ?
Also, document that it's pstrdup'd.

+/*
+ * Parse 'value' as an integer and return the result.
+ *
+ * If parsing fails, set result->parse_error to an appropriate message
+ * and return -1.
+ */
+static int
+expect_integer_value(char *keyword, char *value, bc_specification *result)

-1 isn't great, since it's also an integer, and, also a valid compression level
for zstd (did you see my message about that?).  Maybe INT_MIN is ok.

+{
+   int ivalue;
+   char   *ivalue_endp;
+
+   ivalue = strtol(value, _endp, 10);

Should this also set/check errno ?
And check if value != ivalue_endp ?
See strtol(3)

+char *
+validate_bc_specification(bc_specification *spec)
...
+   /*
+* If a compression level was specified, check that the algorithm 
expects
+* a compression level and that the level is within the legal range for
+* the algorithm.

It would be nice if this could be shared with wal_compression and pg_dump.
We shouldn't need multiple places with structures giving the algorithms and
range of compression levels.

+   unsignedoptions;/* OR of 
BACKUP_COMPRESSION_OPTION constants */

Should be "unsigned int" or "bits32" ?

The server crashes if I send an unknown option - you should hit that in the
regression tests.

$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp 
--no-sync --no-manifest 

Re: ICU for global collation

[Peter Geoghegan]

On Thu, Mar 17, 2022 at 6:15 AM Peter Eisentraut
 wrote:
> committed, thanks

Glad that this finally happened. Thanks to everybody involved!

-- 
Peter Geoghegan

Re: Proposal: Support custom authentication methods using hooks

[Jacob Champion]

On Thu, 2022-03-17 at 14:03 -0400, Stephen Frost wrote:
> * Andres Freund (and...@anarazel.de) wrote:
> > 
> > trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> > possible. I think sspi is doable as well, but I don't know it well enough to
> > be confident. gss without transport encryption could have as well, I
> > think. Even scram-sha-256 looks possible, although that'd have been a good 
> > bit
> > harder.  Where do you see the problems?
> 
> I agree with Peter and don't feel like the above actually answered the
> question in any fashion.  The question wasn't "which auth methods could
> be implemented using these new set of hooks?" but rather- which could
> have been implemented using a plugin mechanism that was designed
> *before* the new method was considered?  The answer to that is
> 'basically none'.  There even existed a plugin mechanism (PAM) before
> many of them, and they weren't able implemented using it.  That's
> entirely the point- while this might be an interesting way to redesign
> what we have now, should we feel that's useful to do (I don't and think
> it would be an actively bad thing for the project to try and do...)
> there's no reason to believe that it'll actually be useful for the
> *next* auth method that comes along.

We already have the beginnings of a SASL framework; part of the work we
did with the OAUTHBEARER experiment was to generalize more pieces of
it. More generalization can still be done. Samay has shown that one
SASL mechanism can be ported, I'm in the process of porting another,
and we already know that the system can handle a non-SASL (password-
based) mechanism.

So that's a pretty good start.

> > Only stuff tying into transport encryption is clearly not doable via the
> > proposed API, but that's hardly the fault of an authentication API?
> 
> This is absolutely the wrong way to look at it.  The authentication
> methods that are coming along today are designed to tie into the
> transport encryption because that's *needed* to avoid MITM attacks.  I'd
> much rather we generally avoid including ones that don't support that
> and we certainly shouldn't be trying to build a generic system which
> explicitly doesn't support it.

Many SASL mechanisms support channel binding, which the server already
has helpers for, and which SCRAM is already using. At least one SASL
mechanism (GSSAPI) supports the negotiation of transport
confidentiality.

These are answerable questions. I don't think it's fair to ask Samay to
have perfect answers while simultaneously implementing a bunch of
different requests in code and going through the brainstorming process
alongside us all. (It would be fair if Samay were claiming the design
were finished, but that's not what I've seen so far.)

--Jacob

Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

Hi,

On 2022-03-17 14:03:31 -0400, Stephen Frost wrote:
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > > Looking at the existing authentication methods
> > > 
> > > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> > > 
> > > how many of these could have been implemented using a plugin mechanism 
> > > that
> > > was designed before the new method was considered?  Probably not many.
> > 
> > trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> > possible. I think sspi is doable as well, but I don't know it well enough to
> > be confident. gss without transport encryption could have as well, I
> > think. Even scram-sha-256 looks possible, although that'd have been a good 
> > bit
> > harder.  Where do you see the problems?
> 
> I agree with Peter and don't feel like the above actually answered the
> question in any fashion.  The question wasn't "which auth methods could
> be implemented using these new set of hooks?" but rather- which could
> have been implemented using a plugin mechanism that was designed
> *before* the new method was considered?  The answer to that is
> 'basically none'.

I fail to see how you come to that conclusion.


> There even existed a plugin mechanism (PAM) before many of them, and they
> weren't able implemented using it.

That's nonsensical. PAM is a platform specific API to start with - so it
doesn't make sense to implement additional postgres authentication mechanism
with it. It also wasn't added to postgres as a base mechanism for extensible
authentication. And it's fundamentally restricted in the kind of secret
exchanges that can be implemented with it.


> That's entirely the point- while this might be an interesting way to
> redesign what we have now, should we feel that's useful to do (I don't and
> think it would be an actively bad thing for the project to try and do...)
> there's no reason to believe that it'll actually be useful for the *next*
> auth method that comes along.

What concrete limitation do you see in the API? It basically gives you the
same powers as writing something in auth.c directly. And due to AUTH_REQ_SASL*
we have fairly extensible ways of exchanging authentication data.


> > Only stuff tying into transport encryption is clearly not doable via the
> > proposed API, but that's hardly the fault of an authentication API?
> 
> This is absolutely the wrong way to look at it.  The authentication
> methods that are coming along today are designed to tie into the
> transport encryption because that's *needed* to avoid MITM attacks.  I'd
> much rather we generally avoid including ones that don't support that
> and we certainly shouldn't be trying to build a generic system which
> explicitly doesn't support it.

So you're saying that any authentication API needs to come together with a
runtime extendable secure transport mechanism? That feels like raising the bar
ridiculously into the sky. If we want to add more transport mechanisms - and
I'm not sure we do - that's a very sizable project on its own. And
independent.

Note that you *can* combine existing secure transport mechanisms with the
proposed API. You can use Port->{ssl,gss,peer_cn,...} to query information and
do further openssl etc calls.

Greetings,

Andres Freund

Re: Column Filtering in Logical Replication

[Tomas Vondra]

I pushed the second fix. Interestingly enough, wrasse failed in the
013_partition test. I don't see how that could be caused by this
particular commit, though - see the pgsql-committers thread [1].

I'd like to test & polish the main patch over the weekend, and get it
committed early next week. Unless someone thinks it's definitely not
ready for that ...


[1]
https://www.postgresql.org/message-id/E1nUsch-0008rQ-FW%40gemulon.postgresql.org

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

Re: support for MERGE

[Alvaro Herrera]

On 2022-Mar-17, Alvaro Herrera wrote:

> I'll see what to do about Instrumentation->nfiltered{1,2,3} that was
> complained about by Andres upthread.  Maybe some additional macros will
> help.

This turns out not to be as simple as I expected, mostly because we want
to keep Instrumentation as a node-agnostic struct.  Things are already a
bit wonky with nfiltered/nfiltered2, and the addition of nfiltered3
makes things a lot uglier, and my impulse of using a union to separate
what is used for scans/joins vs. what is used for MERGE results in an
even more node-specific definition rather than the other way around.

Looking at the history I came across this older thread where this was
discussed
https://postgr.es/m/20180409215851.idwc75ct2bzi6tea@alvherre.pgsql
particularly this message from Robert,
https://www.postgresql.org/message-id/CA%2BTgmoaE3R6%3DV0G6zbht2L_LE%2BTsuYuSTPJXjLW%2B9_tpMGubZQ%40mail.gmail.com

I'll keep looking at this in the coming days, see if I can come up with
something sensible.

-- 
Álvaro Herrera   39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Joshua Brindle]

On Thu, Mar 17, 2022 at 12:36 PM Mark Dilger
 wrote:
> > On Mar 17, 2022, at 9:29 AM, Joshua Brindle 
> >  wrote:
> >
> > I hope this patch can be rolled
> > into the contribution from Mark.
>
> Working on it Thanks for the patch!

Great, thanks.

I missed one objectId reference (InvokeObjectDropHookStr), fixed
version attached.


v2-0001-Add-String-object-access-hooks.patch
Description: Binary data

Commitfest Update

[Greg Stark]

So far this commitfest these 10 patches have been marked committed.
That leaves us with 175 "Needs Review" and 28 "Ready for Comitter" so
quite a ways to go ...

* FUNCAPI tuplestore helper function
* parse/analyze API refactoring
* Add wal_compression=zstd
* Add id's to various elements in protocol.sgml
* Fix alter data type of clustered/replica identity columns
* Fix flaky tests when synchronous_commit = off
* Support of time zone patterns - of, tzh and tzm
* Optionally automatically disable subscription on error
* fix race condition between DROP TABLESPACE and checkpointing
* ICU for global collation


In case anyone's looking for inspiration... Here are the list of
patches marked Ready for Committer:

* document the need to analyze partitioned tables
* ExecTypeSetColNames is fundamentally broken
* pg_dump - read data for some options from external file
* Add comment about startup process getting a free procState array slot always
* Doc patch for retryable xacts
* Function to log backtrace of postgres processes
* Allow multiple recursive self-references
* Expose get_query_def()
* Add checkpoint and redo LSN to LogCheckpointEnd log message
* Fast COPY FROM command for the foreign tables
* Consider parallel for LATERAL subqueries having LIMIT/OFFSET
* Faster pglz compression
* Full support for index LP_DEAD hint bits on standby
* KnownAssignedXidsGetAndSetXmin performance
* Parameter for planner estimates of recursive queries
* enhancing plpgsql API for debugging and tracing
* Make message at end-of-recovery less scary
* Identify missing publications from publisher while create/alter subscription.
* Allow providing restore_command as a command line option to pg_rewind
* Avoid erroring out when unable to remove or parse logical rewrite
files to save checkpoint work
* Allow batched insert during cross-partition updates
* Add callback table access method to reset filenode when dropping relation
* use has_privs_for_role for predefined roles
* range_agg with multirange inputs
* Add new reloption to views for enabling row level security
* Fix pg_rewind race condition just after promotion
* Mitigate pg_rewind race condition, if config file is enlarged concurrently.
* remove exclusive backup mode

-- 
greg

Re: psql - add SHOW_ALL_RESULTS option

[Fabien COELHO]

Hello Peter,


See attached v16 which removes the libpq workaround.


I suppose this depends on

https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com

getting committed, because right now this makes the psql TAP tests fail 
because of the duplicate error message.


How should we handle that?


Ok, it seems I got the patch wrong.

Attached v17 is another try. The point is to record the current status, 
whatever it is, buggy or not, and to update the test when libpq fixes 
things, whenever this is done.



In this part of the patch, there seems to be part of a sentence missing:


Indeed! The missing part was put back in v17.

--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
 SELECT 2.0 AS "float" \;
 SELECT 'world' AS "text" \;
 COMMIT;
+ float 
+---
+   2.0
+(1 row)
+
+ text  
+---
+ world
+(1 row)
+
 -- compound with empty statements and spurious leading spacing
 \;\;   SELECT 3 + 3 \;\;\;   SELECT ' ' || ' !' \;\;   SELECT 1 + 4 \;;
+ ?column? 
+--
+6
+(1 row)
+
+ ?column? 
+--
+   !
+(1 row)
+
  ?column? 
 --
 5
@@ -61,6 +81,11 @@ COMMIT;
 SELECT 1 + 1 + 1 AS "add" \gset
 SELECT :add + 1 + 1 AS "add" \;
 SELECT :add + 1 + 1 AS "add" \gset
+ add 
+-
+   5
+(1 row)
+
 -- set operator
 SELECT 1 AS i UNION SELECT 2 ORDER BY i;
  i 
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions.  (See 
for more details about how the server handles multi-query strings.)
-   Also, psql only prints the
-   result of the last SQL command in the string.
-   This is different from the behavior when the same string is read from
-   a file or fed to psql's standard input,
-   because then psql sends
-   each SQL command separately.
   
   
-   Because of this behavior, putting more than one SQL command in a
-   single -c string often has unexpected results.
-   It's better to use repeated -c commands or feed
-   multiple commands to psql's standard input,
+   If having several commands executed in one transaction is not desired, 
+   use repeated -c commands or feed multiple commands to
+   psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
 
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
 commands included in the string to divide it into multiple
 transactions.  (See 
 for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
 
 
   
@@ -4160,6 +4149,18 @@ bar
   
 
   
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them.  The default is on.  The off behavior
+is for compatibility with older versions of psql.
+
+
+  
+
+   
 SHOW_CONTEXT
 
 
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..2f3dd91602 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
 static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
 static bool command_no_begin(const char *query);
 static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+	bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
 
 
 /*
@@ -354,7 +356,7 @@ CheckConnection(void)
  * Returns true for valid result, false for error state.
  */
 static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
 {
 	bool		OK;
 
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
 break;
 		}
 
-	if (!OK)
+	if (!OK && show_error)
 	{
 		const char *error = PQerrorMessage(pset.db);
 
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
 	}
 }
 
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+	PGresult	*result;
+
+	while ((result = PQgetResult(pset.db)) != NULL)
+		ClearOrSaveResult(result);
+}
+
 
 /*
  * Print microtiming output.  Always 

Re: Proposal: Support custom authentication methods using hooks

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > Looking at the existing authentication methods
> > 
> > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> > 
> > how many of these could have been implemented using a plugin mechanism that
> > was designed before the new method was considered?  Probably not many.
> 
> trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> possible. I think sspi is doable as well, but I don't know it well enough to
> be confident. gss without transport encryption could have as well, I
> think. Even scram-sha-256 looks possible, although that'd have been a good bit
> harder.  Where do you see the problems?

I agree with Peter and don't feel like the above actually answered the
question in any fashion.  The question wasn't "which auth methods could
be implemented using these new set of hooks?" but rather- which could
have been implemented using a plugin mechanism that was designed
*before* the new method was considered?  The answer to that is
'basically none'.  There even existed a plugin mechanism (PAM) before
many of them, and they weren't able implemented using it.  That's
entirely the point- while this might be an interesting way to redesign
what we have now, should we feel that's useful to do (I don't and think
it would be an actively bad thing for the project to try and do...)
there's no reason to believe that it'll actually be useful for the
*next* auth method that comes along.

> Only stuff tying into transport encryption is clearly not doable via the
> proposed API, but that's hardly the fault of an authentication API?

This is absolutely the wrong way to look at it.  The authentication
methods that are coming along today are designed to tie into the
transport encryption because that's *needed* to avoid MITM attacks.  I'd
much rather we generally avoid including ones that don't support that
and we certainly shouldn't be trying to build a generic system which
explicitly doesn't support it.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Proposal: Support custom authentication methods using hooks

[Andres Freund]

Hi,

On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> Looking at the existing authentication methods
> 
> # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> 
> how many of these could have been implemented using a plugin mechanism that
> was designed before the new method was considered?  Probably not many.

trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
possible. I think sspi is doable as well, but I don't know it well enough to
be confident. gss without transport encryption could have as well, I
think. Even scram-sha-256 looks possible, although that'd have been a good bit
harder.  Where do you see the problems?

Only stuff tying into transport encryption is clearly not doable via the
proposed API, but that's hardly the fault of an authentication API?

Greetings,

Andres Freund

Re: Allow async standbys wait for sync replication

[Bharath Rupireddy]

On Sat, Mar 5, 2022 at 1:26 AM Nathan Bossart  wrote:
>
> My point is that there are existing tools for alerting processes when an
> LSN is synchronously replicated and for waking up WAL senders.  What I am
> proposing wouldn't involve spinning in XLogSendPhysical() waiting for
> synchronous replication.  Like SyncRepWaitForLSN(), we'd register our LSN
> in the queue (SyncRepQueueInsert()), but we wouldn't sit in a separate loop
> waiting to be woken.  Instead, SyncRepWakeQueue() would eventually wake up
> the WAL sender and trigger another iteration of WalSndLoop().

While we continue to discuss the other better design at [1], FWIW, I
would like to share a simpler patch that lets wal senders serving
async standbys wait until sync standbys report the flush lsn.
Obviously this is not an elegant way to solve the problem reported in
this thread, as I have this patch ready long back, I wanted to share
it here.

Nathan, of course, this is not something you wanted.

[1] 
https://www.postgresql.org/message-id/CALj2ACWCj60g6TzYMbEO07ZhnBGbdCveCrD413udqbRM0O59RA%40mail.gmail.com

Regards,
Bharath Rupireddy.
From 8a74dbd8c3fe3631b6a2ecee3d8c7a1c98429341 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Thu, 17 Mar 2022 17:01:53 +
Subject: [PATCH v2] Allow async standbys wait for sync replication

---
 doc/src/sgml/config.sgml  |  22 +++
 src/backend/replication/syncrep.c |  11 +-
 src/backend/replication/walsender.c   | 157 ++
 src/backend/utils/misc/guc.c  |   9 +
 src/backend/utils/misc/postgresql.conf.sample |   3 +
 src/include/replication/syncrep.h |   7 +
 src/include/replication/walsender.h   |   1 +
 7 files changed, 202 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7a48973b3c..026e12f5ef 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4247,6 +4247,28 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
   
  
 
+ 
+  async_standbys_wait_for_sync_replication (boolean)
+  
+   async_standbys_wait_for_sync_replication configuration parameter
+  
+  
+  
+   
+When set, asynchronous standbys will wait until synchronous standbys
+(that are either in quorum or priority based) receive WAL, flush and
+acknowledge primary with the flush LSN. This behvaiour is particularly
+important in the event of failover as it avoids manual steps required
+on the asynchronous standbys that might go ahead of the synchronous
+standbys.
+   
+   
+This parameter can only be set in the postgresql.conf
+file or on the server command line.
+   
+  
+ 
+
  
 
 
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index ce163b99e9..398c11579c 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -97,8 +97,6 @@ static bool announce_next_takeover = true;
 SyncRepConfigData *SyncRepConfig = NULL;
 static int	SyncRepWaitMode = SYNC_REP_NO_WAIT;
 
-static void SyncRepQueueInsert(int mode);
-static void SyncRepCancelWait(void);
 static int	SyncRepWakeQueue(bool all, int mode);
 
 static bool SyncRepGetSyncRecPtr(XLogRecPtr *writePtr,
@@ -120,9 +118,6 @@ static int	SyncRepGetStandbyPriority(void);
 static int	standby_priority_comparator(const void *a, const void *b);
 static int	cmp_lsn(const void *a, const void *b);
 
-#ifdef USE_ASSERT_CHECKING
-static bool SyncRepQueueIsOrderedByLSN(int mode);
-#endif
 
 /*
  * ===
@@ -335,7 +330,7 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
  * Usually we will go at tail of queue, though it's possible that we arrive
  * here out of order, so start at tail and work back to insertion point.
  */
-static void
+void
 SyncRepQueueInsert(int mode)
 {
 	PGPROC	   *proc;
@@ -368,7 +363,7 @@ SyncRepQueueInsert(int mode)
 /*
  * Acquire SyncRepLock and cancel any wait currently in progress.
  */
-static void
+void
 SyncRepCancelWait(void)
 {
 	LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
@@ -979,7 +974,7 @@ SyncRepUpdateSyncStandbysDefined(void)
 }
 
 #ifdef USE_ASSERT_CHECKING
-static bool
+bool
 SyncRepQueueIsOrderedByLSN(int mode)
 {
 	PGPROC	   *proc = NULL;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 2d0292a092..97d32a6294 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -125,6 +125,8 @@ int			wal_sender_timeout = 60 * 1000; /* maximum time to send one WAL
 			 * data message */
 bool		log_replication_commands = false;
 
+bool		async_standbys_wait_for_sync_replication = true;
+
 /*
  * State for WalSndWakeupRequest
  */
@@ -258,6 +260,8 @@ static bool TransactionIdInRecentPast(TransactionId xid, uint32 epoch);
 static void WalSndSegmentOpen(XLogReaderState 

Re: Optimize external TOAST storage

[Robert Haas]

On Wed, Mar 16, 2022 at 2:36 PM Nathan Bossart  wrote:
> Thinking further, is simply reducing the number of TOAST chunks the right
> thing to look at?  If I want to add a TOAST attribute that requires 100,000
> chunks, and you told me that I could save 10% in the read path for an extra
> 250 chunks of disk space, I would probably choose read performance.  If I
> wanted to add 100,000 attributes that were each 3 chunks, and you told me
> that I could save 10% in the read path for an extra 75,000 chunks of disk
> space, I might choose the extra disk space.  These are admittedly extreme
> (and maybe even impossible) examples, but my point is that the amount of
> disk space you are willing to give up may be related to the size of the
> attribute.  And maybe one way to extract additional read performance with
> this optimization is to use a variable threshold so that we are more likely
> to use it for large attributes.

Right, so perhaps the ultimate thing here would be a more fine-grained
knob than SET STORAGE EXTERNAL -- something that allows you to specify
that you want to compress only when it really helps. While some people
might find that useful, I think the current patch is less ambitious,
and I think that's OK. It just wants to save something in the cases
where it's basically free. Unfortunately we've learned that it's never
*entirely* free because making the last TOAST chunk longer can always
cost you something, even if it gets longer by only 1 byte. But for
larger values it's hard for that to be significant.

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

Re: Unhyphenation of crash-recovery

[Robert Haas]

On Thu, Mar 17, 2022 at 2:42 AM Peter Eisentraut
 wrote:
> On 16.03.22 02:25, Kyotaro Horiguchi wrote:
> > Hello, this is a derived topic from [1], summarized as $SUBJECT.
> >
> > This just removes useless hyphens from the words
> > "(crash|emergency)-recovery". We don't have such wordings for "archive
> > recovery" This patch fixes non-user-facing texts as well as
> > user-facing ones.
>
> Most changes in this patch are not the correct direction.  The hyphens
> are used to group compound adjectives before nouns.  For example,
>
>  simple crash-recovery cases
>
> means
>
>  simple (crash recovery) cases
>
> rather than
>
>  simple crash (recovery cases)
>
> if it were without hyphens.

I agree with that as a general principle, but I also think the
particular case you've chosen here is a good example of another
principle: sometimes it just doesn't matter very much. A case of crash
recovery that happens to be simple is pretty much the same thing as a
case of recovery that is simple and involves a crash. My understanding
of English grammar is that one typically does not hyphenate unless it
is required to avoid confusion. A quick Google search suggests this
example:

Mr Harper had a funny smelling dog

We must try to figure out whether the smell of the dog is funny or
whether the dog itself is both funny and smelling. If we hyphenate
funny-smelling, then it's clear that it is the smell of the dog that
is funny and not the dog itself. But in your example I cannot see that
there is any similar ambiguity. Recovery cases can involve a crash,
and crash recovery can have cases, and what's the difference, anyway?
So I wouldn't hyphenate it, but I also wouldn't spend a lot of time
arguing if someone else did. Except maybe that's exactly what I am
doing. Perhaps I should find something else to do.

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

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Mark Dilger]

> On Mar 17, 2022, at 9:29 AM, Joshua Brindle  
> wrote:
> 
> I hope this patch can be rolled
> into the contribution from Mark.

Working on it Thanks for the patch!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Robert Haas]

On Thu, Mar 17, 2022 at 12:30 PM Joshua Brindle
 wrote:
> I agree with this view, outside of the mixup between MAC and DAC (DAC
> is in-core, MAC is via hooks)

An excellent point! Exactly why we need expert-level help with this stuff! :-)

> So there should be some committers or contributors that keep an eye
> out and make sure new objects have appropriate hooks, and since an Oid
> based hook for GUCs is inappropriate, I hope this patch can be rolled
> into the contribution from Mark.

I'm not going to take a position on this patch right this minute, but
I appreciate you providing it.

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

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Joshua Brindle]

On Thu, Mar 17, 2022 at 12:04 PM Robert Haas  wrote:
>
> On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle
>  wrote:
> > 
> >
> > > I remain of the opinion that this
> > > patch should not concern itself with that, though.
> >
> > So you are saying that people can add new object types to PG with DAC
> > permissions and not concern themselves with MAC capable hooks? Is that
> > an official PG community stance?
>
> I don't know that the community has an official position on that
> topic, but I do not think it's reasonable to expect everyone who
> tinkers with MAC permissions to try to make a corresponding equivalent
> for DAC. The number of people using PostgreSQL with DAC is relatively
> small, and the topic is extremely complicated, and a lot of hackers
> don't really understand it well enough to be sure that whatever they
> might do is right. I think it's reasonable to expect people who
> understand DAC and care about it to put some energy into the topic,
> and not just in terms of telling other people how they have to write
> their patches.
>
> I *don't* think it's appropriate for a patch that touches MAC to
> deliberately sabotage the existing support we have for DAC or to just
> ignore it where the right thing to do is obvious. But maintaining a
> million lines of code is a lot of work, and I can't think of any
> reason why the burden of maintaining relatively little-used features
> should fall entirely on people who don't care about them.

I agree with this view, outside of the mixup between MAC and DAC (DAC
is in-core, MAC is via hooks)

So there should be some committers or contributors that keep an eye
out and make sure new objects have appropriate hooks, and since an Oid
based hook for GUCs is inappropriate, I hope this patch can be rolled
into the contribution from Mark.

The attached patch adds a set of hooks for strings, and changes the
GUC patch from Mark to use it, I tested with this code:

void set_user_object_access_str(ObjectAccessType access, Oid classId,
const char *objName, int subId, void *arg)
{
if (next_object_access_hook_str)
{
(*next_object_access_hook_str)(access, classId, objName, subId, arg);
}
switch (access)
{
case OAT_POST_ALTER:
if (classId == SettingAclRelationId)
{
Oid userid = GetUserId();
bool is_superuser = superuser_arg(userid);
char *username = GETUSERNAMEFROMID(GetUserId());
const char *setting = "setting value";
const char *altering = "altering system";
const char *unknown = "unknown";
const char *action;
if (subId && ACL_SET_VALUE)
action = setting;
else if (subId && ACL_ALTER_SYSTEM)
action = altering;
else
action = unknown;

elog(WARNING, "%s is %s %s (%d)", username, action, objName, is_superuser);
if (strcmp(objName, "log_statement") == 0)
{
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("Setting %s blocked", objName)));
}
}
default:
break;
}
}

static void
set_user_object_access (ObjectAccessType access, Oid classId, Oid
objectId, int subId, void *arg)
{
if (next_object_access_hook)
{
(*next_object_access_hook)(access, classId, objectId, subId, arg);
}
switch (access)
{
case OAT_FUNCTION_EXECUTE:
{
/* Update the `set_config` Oid cache if necessary. */
set_user_cache_proc(InvalidOid);

/* Now see if this function is blocked */
set_user_block_set_config(objectId);
break;
}

/* fallthrough */

case OAT_POST_CREATE:
{
if (classId == ProcedureRelationId)
{
set_user_cache_proc(objectId);
}
break;
}
default:
break;
}
}


0001-Add-String-object-access-hooks.patch
Description: Binary data

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Robert Haas]

On Thu, Mar 17, 2022 at 12:19 PM Mark Dilger
 wrote:
> Joshua helped test the DAC portion of this patch, and answered a number of my 
> questions on the topic, including in-person back in December.  I take your 
> point, Robert, on the general principle, but the archives should reflect that 
> Joshua did contribute to this specific patch.

I wasn't intending to say otherwise, and if the changes needed for DAC
here are straightforward and don't make the patch significantly harder
to finish, then I would say Tom is wrong and we should just go ahead
and make them. But if they do, then I think it's perfectly fine to say
that we're going to leave that alone and let someone with
subject-matter expertise sort it out when they have time. We do that
all the time with other things, most notably MSVC builds, where we
just can't expect every hacker or even every committer to understand
exactly what's required to make it all work. I try my best to commit
things that don't break it, but sometimes I do, and then Andrew helps
sort it out, because he understands it and I don't. I think DAC should
fall into that kind of category as well.

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

Re: Support logical replication of DDLs

[Zheng Li]

Hello Alvaro,

> I think this is a pretty interesting and useful feature.
>
> Did you see some old code I wrote towards this goal?
> https://www.postgresql.org/message-id/20150215044814.gl3...@alvh.no-ip.org
> The intention was that DDL would produce some JSON blob that accurately
> describes the DDL that was run; the caller can acquire that and use it
> to produce working DDL that doesn't depend on runtime conditions.  There
> was lots of discussion on doing things this way.  It was ultimately
> abandoned, but I think it's valuable.

Thanks for pointing this out. I'm looking into it.


Hello Japin,
>The new patch change the behavior of publication, when we drop a table
>on publisher, the table also be dropped on subscriber, and this made the
>regression testa failed, since the regression test will try to drop the
>table on subscriber.

>IMO, we can disable the DDLs replication by default, which makes the
>regression test happy.  Any thoughts?

Good catch, I forgot to run the whole TAP test suite. I think for now
we can simply disable DDL replication for the failing tests when
creating publication: $node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
I'll fix it in the next version of patch. I'll also work on breaking
down the patch into smaller pieces for ease of review.

Regards,
Zheng

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Mark Dilger]

> On Mar 17, 2022, at 9:04 AM, Robert Haas  wrote:
> 
> not just in terms of telling other people how they have to write
> their patches.

...

>  the burden of maintaining relatively little-used features
> should fall entirely on people who don't care about them.

Joshua helped test the DAC portion of this patch, and answered a number of my 
questions on the topic, including in-person back in December.  I take your 
point, Robert, on the general principle, but the archives should reflect that 
Joshua did contribute to this specific patch.

Joshua, should we drop the DAC portion for postgres 15 and revisit the issue 
for postgres 16?  I think it's getting late in the development cycle to attempt 
what Tom described upthread, and I'd hate to see the rest of this patch punted.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Robert Haas]

On Wed, Mar 16, 2022 at 2:47 PM Tom Lane  wrote:
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables?  I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters".  Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning.  But you can't
> GRANT a parameter's current value.

I agree that the lack of a good user-friendly term for GUCs is a real
problem. Here at EDB I've observed even relatively non-technical
people using that term, which appears nowhere in the documentation and
is utterly unintelligible to a typical end-user. Somebody gets on the
phone and tells the customer that they need to set a GUC and the
customer is like "what's a guck?" except that they probably don't
actually ask that question but are just confused and fail to
understand that a postgresql.conf change is being proposed. I hate it.
It sucks.

I have sort of been trying to promote the use of the word "setting"
and use it in my own writing, especially to end-users. That is
definitely more intelligible to random users, but it's admittedly also
awkward. "Set a setting" just sounds redundant. But "set a
configuration variable" sounds wordy, so I don't know.

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

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Robert Haas]

On Thu, Mar 17, 2022 at 9:25 AM Joshua Brindle
 wrote:
> 
>
> > I remain of the opinion that this
> > patch should not concern itself with that, though.
>
> So you are saying that people can add new object types to PG with DAC
> permissions and not concern themselves with MAC capable hooks? Is that
> an official PG community stance?

I don't know that the community has an official position on that
topic, but I do not think it's reasonable to expect everyone who
tinkers with MAC permissions to try to make a corresponding equivalent
for DAC. The number of people using PostgreSQL with DAC is relatively
small, and the topic is extremely complicated, and a lot of hackers
don't really understand it well enough to be sure that whatever they
might do is right. I think it's reasonable to expect people who
understand DAC and care about it to put some energy into the topic,
and not just in terms of telling other people how they have to write
their patches.

I *don't* think it's appropriate for a patch that touches MAC to
deliberately sabotage the existing support we have for DAC or to just
ignore it where the right thing to do is obvious. But maintaining a
million lines of code is a lot of work, and I can't think of any
reason why the burden of maintaining relatively little-used features
should fall entirely on people who don't care about them.

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

Re: Support logical replication of DDLs

[Japin Li]

On Thu, 17 Mar 2022 at 05:17, Zheng Li  wrote:
> Hi,
>
>>If you don't mind, would you like to share the POC or the branch for this 
>>work?
>
> The POC patch is attached. It currently supports the following 
> functionalities:

Hi,

When I try to run regression test, there has some errors.

make[2]: Entering directory 
'/home/px/Codes/postgresql/Debug/src/test/subscription'
rm -rf '/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && 
/usr/bin/mkdir -p 
'/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && cd 
/home/px/Codes/postgresql/Debug/../src
/test/subscription && 
TESTDIR='/home/px/Codes/postgresql/Debug/src/test/subscription' 
PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/bin:/home/px/Codes/postgresql/Debu
g/src/test/subscription:$PATH" 
LD_LIBRARY_PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/lib:$LD_LIBRARY_PATH"
  PGPORT='65432' PG_REGRESS='/home/px/Codes/postgresql/De
bug/src/test/subscription/../../../src/test/regress/pg_regress' /usr/bin/prove 
-I /home/px/Codes/postgresql/Debug/../src/test/perl/ -I 
/home/px/Codes/postgresql/Debug/../src/test/subscription  t/*.pl
t/001_rep_changes.pl ... ok
t/002_types.pl . ok
t/003_constraints.pl ... ok
t/004_sync.pl .. 6/? # Tests were run but no plan was 
declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 7.
t/004_sync.pl .. Dubious, test returned 29 (wstat 7424, 
0x1d00)
All 7 subtests passed
t/005_encoding.pl .. ok
t/006_rewrite.pl ... 1/? # poll_query_until timed out executing 
this query:
# SELECT '0/14A8648' <= replay_lsn AND state = 'streaming' FROM 
pg_catalog.pg_stat_replication WHERE application_name = 'mysub';
# expecting this output:
# t
# last actual query output:
#
# with stderr:
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 1.
t/006_rewrite.pl ... Dubious, test returned 29 (wstat 7424, 
0x1d00)
All 1 subtests passed
t/007_ddl.pl ... ok
t/008_diff_schema.pl ... 1/? # Tests were run but no plan was 
declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 4.
t/008_diff_schema.pl ... Dubious, test returned 29 (wstat 7424, 
0x1d00)
All 4 subtests passed
t/009_matviews.pl .. Dubious, test returned 29 (wstat 7424, 
0x1d00)
No subtests run

The new patch change the behavior of publication, when we drop a table
on publisher, the table also be dropped on subscriber, and this made the
regression testa failed, since the regression test will try to drop the
table on subscriber.

IMO, we can disable the DDLs replication by default, which makes the
regression test happy.  Any thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: refactoring basebackup.c (zstd workers)

[Robert Haas]

On Mon, Mar 14, 2022 at 1:21 PM Robert Haas  wrote:
> There's some appeal to that, but one downside is that it means that
> the client can't be used to fetch data that is compressed in a way
> that the server knows about and the client doesn't. I don't think
> that's great. Why should, for example, pg_basebackup need to be
> compiled with zstd support in order to request zstd compression on the
> server side? If the server knows about the brand new
> justin-magic-sauce compression algorithm, maybe the client should just
> be able to request it and, when given various .jms files by the
> server, shrug its shoulders and accept them for what they are. That
> doesn't work if -Fp is involved, or similar, but it should work fine
> for simple cases if we set things up right.

Concretely, I propose the attached patch for v15. It renames the
newly-added COMPRESSION_LEVEL option to COMPRESSION_DETAIL, introduces
a flexible syntax for options along the lines you proposed, and
adjusts things so that a client that doesn't support a particular type
of compression can still request that type of compression from the
server.

I think it's important to do this for v15 so that we don't end up with
backward-compatibility problems down the road.

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


v1-0001-Replace-BASE_BACKUP-COMPRESSION_LEVEL-option-with.patch
Description: Binary data

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Andrew Dunstan]

On 3/17/22 10:47, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 16.03.22 19:47, Tom Lane wrote:
>>> ...  Perhaps we could just use "SET" and
>>> "ALTER", or "SET" and "SYSTEM"?
>> I think Oracle and MS SQL Server have many multi-word privilege names. 
>> So users are quite used to that.  And if we want to add more complex 
>> privileges, we might run out of sensible single words eventually.  So I 
>> would not exclude this approach.
> Well, I still say that "SET" is sufficient for the one privilege name
> (unless we really can't make Bison handle that, which I doubt).  But
> I'm willing to yield on using "ALTER SYSTEM" for the other.
>
> If we go with s/SETTING/PARAMETER/ as per your other message, then
> that would be adequately consistent with the docs I think.  So it'd
> be
>
> GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...
>
> and the new catalog would be pg_parameter_acl, and so on.
>
>   



The upside of this is that it avoids the inelegant


    GRANT SET ON SETTING ...


But I was just looking again at the grammar, and the only reason we need
this keyword at all AFAICS is to disambiguate ALL [PRIVILEGES] cases. 
If we abandoned that for this form of GRANT/REVOKE I think we could
probably get away with


    GRANT { SET | ALTER SYSTEM } ON setting_name ...


I haven't tried it, so I could be all wrong.


cheers


andrew

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

Re: psql - add SHOW_ALL_RESULTS option

[Tom Lane]

Peter Eisentraut  writes:
> I suppose this depends on
> https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com
> getting committed, because right now this makes the psql TAP tests fail 
> because of the duplicate error message.

Umm ... wasn't 618c16707 what you need here?

regards, tom lane

Re: Add 64-bit XIDs into PostgreSQL 15

[Pavel Borisov]

> I forked this thread as requested by several people in the discussion [1].
>
> The new thread contains two patches that are targeting PG15. I replaced
> the thread in the current CF to [1]. This thread was added to the next CF.
> I suggest we continue discussing changes targeting PG >= 16 here.
>
> [1]:
> https://postgr.es/m/caj7c6tpdoybyrncaeyndkbkto0wg2xsdydutf0nxq+vfkmt...@mail.gmail.com
>
Thanks!
We're planning to add 0001 and 0002 with next v20 to the mentioned [1]
thread to deliver them into v15
In this thread we'll add full patchset v20 as 0003, 0004 and 0005 are not
supposed to be committed without 0001 and 0002 anyway.

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: psql - add SHOW_ALL_RESULTS option

[Peter Eisentraut]

On 12.03.22 17:27, Fabien COELHO wrote:



Are you planning to send a rebased patch for this commit fest?


Argh, I did it in a reply in another thread:-( Attached v15.

So as to help moves things forward, I'd suggest that we should not to 
care too much about corner case repetition of some error messages 
which are due to libpq internals, so I could remove the ugly buffer 
reset from the patch and have the repetition, and if/when the issue is 
fixed later in libpq then the repetition will be removed, fine! The 
issue is that we just expose the strange behavior of libpq, which is 
libpq to solve, not psql.


See attached v16 which removes the libpq workaround.


I suppose this depends on

https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com

getting committed, because right now this makes the psql TAP tests fail 
because of the duplicate error message.


How should we handle that?


In this part of the patch, there seems to be part of a sentence missing:

+ * Marshal the COPY data.  Either subroutine will get the
+ * connection out of its COPY state, then
+ * once and report any error. Return whether all was ok.

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Tom Lane]

Peter Eisentraut  writes:
> On 16.03.22 19:47, Tom Lane wrote:
>> ...  Perhaps we could just use "SET" and
>> "ALTER", or "SET" and "SYSTEM"?

> I think Oracle and MS SQL Server have many multi-word privilege names. 
> So users are quite used to that.  And if we want to add more complex 
> privileges, we might run out of sensible single words eventually.  So I 
> would not exclude this approach.

Well, I still say that "SET" is sufficient for the one privilege name
(unless we really can't make Bison handle that, which I doubt).  But
I'm willing to yield on using "ALTER SYSTEM" for the other.

If we go with s/SETTING/PARAMETER/ as per your other message, then
that would be adequately consistent with the docs I think.  So it'd
be

GRANT { SET | ALTER SYSTEM } ON PARAMETER foo TO ...

and the new catalog would be pg_parameter_acl, and so on.

regards, tom lane

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Pavel Borisov]

>
> On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov 
> wrote:
> > On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
> >  wrote:
> >> On 17.03.22 14:12, Aleksander Alekseev wrote:
> >> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
> >> > refactoring allows us to switch to UINT64_FORMAT by changing one
> >> > #define in the future patches.
> >> >
> >> > Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
> >> > xid)` instead in order to simplify localization of the error messages.
> >> > Personally I don't have a strong opinion here. Either approach will
> >> > work and will affect the error messages eventually. Please let us know
> >> > what you think.
> >>
> >> This is not a question of simplification.  Translatable messages with
> >> embedded macros won't work.  This patch isn't going to be acceptable.
> >
> > I've suspected this, but wasn't sure.  Thank you for clarification.
>
Hi, hackers!

The need to support localization is very much understood by us. We'll
deliver a patchset soon with localization based on %lld/%llu format and
explicit casts to unsigned/signed long long.
Alexander Alexeev could you wait a little bit and give us time to deliver
v20 patch which will address localization (I propose concurrent work should
stop until that to avoid conflicts)
Advice and discussion help us a lot.

Thanks!


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Tom Lane]

Japin Li  writes:
> Maybe, we should format it to string before for localization messages,
> like the following code snippet comes from pg_backup_tar.c.
> However, I do not think it is a good way.

> snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
> snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
> fatal("actual file length (%s) does not match expected (%s)",
>   buf1, buf2);

That used to be our standard practice before we switched to always
relying on our own snprintf.c.  Now, we know that "%lld" with an
explicit cast to long long will work, so that's the preferred method
for printing 64-bit values in localizable strings.  Not all of the old
code has been updated, though.

regards, tom lane

Re: Column Filtering in Logical Replication

[Peter Eisentraut]

I notice that the publication.sql regression tests contain a number of 
comments like


+-- error: replica identity "a" not included in the column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (b, c);

but the error doesn't actually happen, because of the way the replica 
identity checking was changed.  This needs to be checked again.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Japin Li]

On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov  wrote:
> On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
>  wrote:
>> On 17.03.22 14:12, Aleksander Alekseev wrote:
>> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
>> > refactoring allows us to switch to UINT64_FORMAT by changing one
>> > #define in the future patches.
>> >
>> > Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
>> > xid)` instead in order to simplify localization of the error messages.
>> > Personally I don't have a strong opinion here. Either approach will
>> > work and will affect the error messages eventually. Please let us know
>> > what you think.
>>
>> This is not a question of simplification.  Translatable messages with
>> embedded macros won't work.  This patch isn't going to be acceptable.
>
> I've suspected this, but wasn't sure.  Thank you for clarification.
>

Maybe, we should format it to string before for localization messages,
like the following code snippet comes from pg_backup_tar.c.
However, I do not think it is a good way.

snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
fatal("actual file length (%s) does not match expected (%s)",
  buf1, buf2);

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Peter Eisentraut]

On 16.03.22 19:47, Tom Lane wrote:

I'm also fairly allergic to the way that this patch has decided to assign
multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM).  There
is no existing precedent for that, and I think it's going to break
client-side code that we don't need to break.  It's not coincidental that
this forces weird changes in rules about whitespace in the has_privilege
functions, for example; and if you think that isn't going to cause
problems I think you are wrong.  Perhaps we could just use "SET" and
"ALTER", or "SET" and "SYSTEM"?


I think Oracle and MS SQL Server have many multi-word privilege names. 
So users are quite used to that.  And if we want to add more complex 
privileges, we might run out of sensible single words eventually.  So I 
would not exclude this approach.

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Peter Eisentraut]

On 16.03.22 19:59, Mark Dilger wrote:

Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the 
two obvious choices.  I preferred "configuration parameter" originally and was argued out of it.  My take on "setting" was also that it 
more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter 
"work_mem" has the setting "8192".


"The current setting of the work_mem parameter is 8192."

I think something based on "parameter" is good.  We also use that 
language in the protocol (e.g., ParameterStatus).

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Alexander Korotkov]

On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
 wrote:
> On 17.03.22 14:12, Aleksander Alekseev wrote:
> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
> > refactoring allows us to switch to UINT64_FORMAT by changing one
> > #define in the future patches.
> >
> > Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
> > xid)` instead in order to simplify localization of the error messages.
> > Personally I don't have a strong opinion here. Either approach will
> > work and will affect the error messages eventually. Please let us know
> > what you think.
>
> This is not a question of simplification.  Translatable messages with
> embedded macros won't work.  This patch isn't going to be acceptable.

I've suspected this, but wasn't sure.  Thank you for clarification.

--
Regards,
Alexander Korotkov

Re: Out-of-tree certificate interferes ssltest

[Daniel Gustafsson]

> On 17 Mar 2022, at 09:05, Kyotaro Horiguchi  wrote:
> 
> At Thu, 17 Mar 2022 16:22:14 +0900, Michael Paquier  
> wrote in 
>> On Thu, Mar 17, 2022 at 02:59:26PM +0900, Michael Paquier wrote:
>>> In both cases, enforcing sslcrl to a value of "invalid" interferes
>>> with the failure scenario we expect from sslcrldir.  It is possible to
>>> bypass that with something like the attached, but that's a kind of
>>> ugly hack.  Another alternative would be to drop those two tests, and
>>> I am not sure how much we care about these two negative scenarios.

I really don't think we should drop tests based on these premises, at least not
until it's raised as a problem/inconvenience but more hackers.  I would prefer
to instead extend the error message with hints that ~/.postgresql contents
could've affected test outcome.  But, as the v2 patch handles it this is mostly
academic at this point.

>> Actually, there is a trick I have recalled here: we can enforce sslcrl
>> to an empty value in the connection string after the default.  This
>> still ensures that the test won't pick up any SSL data from the local
>> environment and avoids any interferences of OpenSSL's
>> X509_STORE_load_locations().  This gives a much simpler and cleaner
>> patch.

> Ah! I didn't have a thought that we can specify the same parameter
> twice.  It looks like clean and robust.  $default_ssl_connstr contains
> all required options in PQconninfoOptions[].

+1

One small concern though. This hunk:

+my $default_ssl_connstr = "sslkey=invalid sslcert=invalid sslrootcert=invalid 
sslcrl=invalid sslcrldir=invalid";
+
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR 
host=common-name.pg-ssltest.test";
+  "$default_ssl_connstr user=ssltestuser dbname=trustdb 
hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
 
..together with the following changes along the lines of:

-   "$common_connstr sslrootcert=invalid sslmode=require",
+   "$common_connstr sslmode=require",

..is making it fairly hard to read the test and visualize what the connection
string is and how the test should behave.  I don't have a better idea off the
top of my head right now, but I think this is an area to revisit and improve
on.

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

Re: Granting SET and ALTER SYSTE privileges for GUCs

[Joshua Brindle]

> I remain of the opinion that this
> patch should not concern itself with that, though.

So you are saying that people can add new object types to PG with DAC
permissions and not concern themselves with MAC capable hooks? Is that
an official PG community stance?

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Peter Eisentraut]

On 17.03.22 14:12, Aleksander Alekseev wrote:

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.


This is not a question of simplification.  Translatable messages with 
embedded macros won't work.  This patch isn't going to be acceptable.

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

[Alexander Korotkov]

Hi, Aleksander!

On Thu, Mar 17, 2022 at 4:12 PM Aleksander Alekseev
 wrote:
> This thread is a fork of [1], created per request by several people in
> the discussion. It includes two patches from the patchset that we
> believe can be delivered in PG15. The rest of the patches are
> targeting PG >= 16 and can be discussed further in [1].

Thank you for putting this into a separate thread.

> v19-0001 changes the format string for XIDs from %u to XID_FMT. This
> refactoring allows us to switch to UINT64_FORMAT by changing one
> #define in the future patches.
>
> Kyotaro suggested using `errmsg("blah blah  %lld ..", (long long)
> xid)` instead in order to simplify localization of the error messages.
> Personally I don't have a strong opinion here. Either approach will
> work and will affect the error messages eventually. Please let us know
> what you think.

I'm not a localization expert.  Could you clarify what localization
messages should look like if we switch to XID_FMT?  And will we have
to change them if change the definition of XID_FMT?

--
Regards,
Alexander Korotkov

Re: Add 64-bit XIDs into PostgreSQL 15

[Aleksander Alekseev]

Hi hackers,

I forked this thread as requested by several people in the discussion [1].

The new thread contains two patches that are targeting PG15. I replaced the
thread in the current CF to [1]. This thread was added to the next CF. I
suggest we continue discussing changes targeting PG >= 16 here.

[1]:
https://postgr.es/m/caj7c6tpdoybyrncaeyndkbkto0wg2xsdydutf0nxq+vfkmt...@mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

[Bharath Rupireddy]

On Thu, Mar 17, 2022 at 12:12 AM Nathan Bossart
 wrote:
>
> On Wed, Mar 16, 2022 at 03:02:41PM +0530, Bharath Rupireddy wrote:
> > On Mon, Mar 14, 2022 at 10:34 PM Nathan Bossart
> >  wrote:
> >> I'm -1 on splitting these new statistics to separate LOGs.  In addition to
> >> making it more difficult to discover statistics for a given checkpoint, I
> >> think it actually adds even more bloat to the server log.  If we are
> >> concerned about the amount of information in these LOGs, perhaps we should
> >> adjust the format to make it more human-readable.
> >
> > Below are the ways that I can think of. Thoughts?
>
> I think I prefer the first option.  If these tasks don't do anything, we
> leave the stats out of the message.  If they do some work, we add them.

Thanks Nathan.

> Below are the ways that I can think of. Thoughts?
>
> 1)
> if (restartpoint)
> {
>if (snap_files_rmvd > 0 && map_files_rmvd > 0)
>  existing "restartpoint complete" message + "snapshot files
> removed cnt, time, cutoff lsn" + "mapping files removed cnt, time,
> cutoff lsn"
>else if (snap_files_rmvd > 0)
>  existing "restartpoint complete" message + "snapshot files
> removed cnt, time, cutoff lsn"
>else if (map_files_rmvd > 0)
>  existing "restartpoint complete" message + "mapping files removed
> cnt, time, cutoff lsn"
>else
>  existing "restartpoint complete" message
> }
> else
> {
>same as above but with "checkpoint complete" instead of "restart complete"
> }
>
> 2) Use StringInfoData to prepare the message dynamically but this will
> create  message translation problems.
>
> 3) Emit the  "snapshot files removed cnt, time, cutoff lsn" and
> "mapping files removed cnt, time, cutoff lsn" at LOG level if
> (log_checkpoints) in CheckPointSnapBuild and
> CheckPointLogicalRewriteHeap respectively.
>
> 4) Have separate log messages in LogCheckpointEnd:
> if (snap_files_rmvd > 0)
>"snapshot files removed cnt, time, cutoff lsn"
> if (map_files_rmvd > 0)
>"mapping files removed cnt, time, cutoff lsn"
>
> if (restartpoint)
>   existing "restartpoint complete" message
> else
>   existing "checkpoint complete" message

FWIW, I'm attaching patches as follows:
v4-0001...patch implements option (4)
v4-0002...txt implements option (3)
v4-0003...txt implements option (1)

Personally, I tend to agree with v4-0001 (option (4)) or v4-0002
(option (3)) than v4-0003 (option (1)) as it adds more unreadability
with most of the code duplicated creating more diff with previous
versions and maintainability problems. Having said that, I will leave
it to the committer to decide on that.

Regards,
Bharath Rupireddy.
From caa4ab307a2daaf6cc495aaea78d34dbd66faa2f Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Thu, 17 Mar 2022 13:06:30 +
Subject: [PATCH v444] Add checkpoint stats of snapshot and mapping files of
 pg_logical dir

At times, there can be many snapshot and mapping files under
pg_logical dir that the checkpoint might have to delete/fsync
based on the cutoff LSN which can increase the checkpoint time.
Add stats related to these files to better understand the delays
or time spent by the checkpointer processing them.

Add these new log message only when necessary i.e. at least one
snapshot or mapping files is processed during the checkpoint.
---
 src/backend/access/heap/rewriteheap.c   | 30 +
 src/backend/replication/logical/snapbuild.c | 25 +
 2 files changed, 55 insertions(+)

diff --git a/src/backend/access/heap/rewriteheap.c 
b/src/backend/access/heap/rewriteheap.c
index 2a53826736..471ad4c5e8 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1196,6 +1196,13 @@ CheckPointLogicalRewriteHeap(void)
DIR*mappings_dir;
struct dirent *mapping_de;
charpath[MAXPGPATH + 20];
+   uint64  files_rmvd_cnt = 0;
+   uint64  files_syncd_cnt = 0;
+   TimestampTz start_t;
+   TimestampTz end_t;
+
+   if (log_checkpoints)
+   start_t = GetCurrentTimestamp();
 
/*
 * We start of with a minimum of the last redo pointer. No new decoding
@@ -1247,6 +1254,9 @@ CheckPointLogicalRewriteHeap(void)
ereport(ERROR,
(errcode_for_file_access(),
 errmsg("could not remove file 
\"%s\": %m", path)));
+
+   if (log_checkpoints)
+   files_rmvd_cnt++;
}
else
{
@@ -1280,9 +1290,29 @@ CheckPointLogicalRewriteHeap(void)
(errcode_for_file_access(),
 errmsg("could not close file 
\"%s\": %m", path)));
}
+
+   if (log_checkpoints)
+   files_syncd_cnt++;
}

Re: ICU for global collation

[Peter Eisentraut]

On 17.03.22 13:01, Shinoda, Noriyoshi (PN Japan FSIP) wrote:

Thank you to all the developers.
I found that the description of the pg_database.daticulocale column was not 
written in the documentation.
The attached small patch adds a description of the daticulocale column to 
catalogs.sgml.


committed, thanks

RE: Skipping logical replication transactions on subscriber side

[osumi.takami...@fujitsu.com]

On Thursday, March 17, 2022 7:56 PM Masahiko Sawada  
wrote:
 On Thu, Mar 17, 2022 at 5:52 PM Amit Kapila 
> wrote:
> >
> > On Thu, Mar 17, 2022 at 12:39 PM osumi.takami...@fujitsu.com
> >  wrote:
> > >
> > > On Thursday, March 17, 2022 3:04 PM Amit Kapila
>  wrote:
> > > > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada
> > > >  wrote:
> > > > >
> > > > > I've attached an updated version patch.
> > > > >
> > > >
> > > > The patch LGTM. I have made minor changes in comments and docs in
> > > > the attached patch. Kindly let me know what you think of the attached?
> > > Hi, thank you for the patch. Few minor comments.
> > >
> > >
> > > (3) apply_handle_commit_internal
> > >
> > ...
> > >
> > > I feel if we move those two functions at the end of the
> > > apply_handle_commit and apply_handle_stream_commit, then we will
> > > have more aligned codes and improve readability.
> > >
> 
> I think we cannot just move them to the end of apply_handle_commit() and
> apply_handle_stream_commit(). Because if we do that, we end up missing
> updating replication_session_origin_lsn/timestamp when clearing the
> subskiplsn if we're skipping a non-stream transaction.
> 
> Basically, the apply worker differently handles 2pc transactions and non-2pc
> transactions; we always prepare even empty transactions whereas we don't
> commit empty non-2pc transactions. So I think we don’t have to handle both in
> the same way.
Okay. Thank you so much for your explanation.
Then the code looks good to me.


Best Regards,
Takamichi Osumi

RE: ICU for global collation

[Shinoda, Noriyoshi (PN Japan FSIP)]

Hi, 
Thank you to all the developers.
I found that the description of the pg_database.daticulocale column was not 
written in the documentation. 
The attached small patch adds a description of the daticulocale column to 
catalogs.sgml.

Regards,
Noriyoshi Shinoda
-Original Message-
From: Peter Eisentraut  
Sent: Thursday, March 17, 2022 7:29 PM
To: Julien Rouhaud 
Cc: pgsql-hackers ; Daniel Verite 

Subject: Re: ICU for global collation

On 15.03.22 08:41, Julien Rouhaud wrote:
 The locale object in ICU is an identifier that specifies a 
 particular locale and has fields for language, country, and an 
 optional code to specify further variants or subdivisions. These 
 fields also can be represented as a string with the fields separated by an 
 underscore.
>>
>> I think the Localization chapter needs to be reorganized a bit, but 
>> I'll leave that for a separate patch.
> 
> WFM.

I ended up writing a bit of content for that chapter.

>>> While on that topic, the doc should probably mention that default 
>>> ICU collations can only be deterministic.
>>
>> Well, there is no option to do otherwise, so I'm not sure where/how 
>> to mention that.  We usually don't document options that don't exist. 
>> ;-)
> 
> Sure, but I'm afraid that users may still be tempted to use ICU 
> locales like
> und-u-ks-level2 from the case_insensitive example in the doc and hope 
> that it will work accordingly.  Or maybe it's just me that still sees 
> ICU as dark magic and want to be extra cautious.

I have added this to the CREATE DATABASE ref page.

> Unrelated, but I just realized that we have PGC_INTERNAL gucs for 
> lc_ctype and lc_collate.  Should we add one for icu_locale too?

I'm not sure.  I think the existing ones are more for backward compatibility 
with the time before it was settable per database.

>>> in AlterCollation(), pg_collation_actual_version(), 
>>> AlterDatabaseRefreshColl() and pg_database_collation_actual_version():
>>>
>>> -   datum = SysCacheGetAttr(COLLOID, tup, Anum_pg_collation_collcollate, 
>>> );
>>> -   Assert(!isnull);
>>> -   newversion = get_collation_actual_version(collForm->collprovider, 
>>> TextDatumGetCString(datum));
>>> +   datum = SysCacheGetAttr(COLLOID, tup, collForm->collprovider == 
>>> COLLPROVIDER_ICU ? Anum_pg_collation_colliculocale : 
>>> Anum_pg_collation_collcollate, );
>>> +   if (!isnull)
>>> +   newversion = get_collation_actual_version(collForm->collprovider, 
>>> TextDatumGetCString(datum));
>>> +   else
>>> +   newversion = NULL;
>>>
>>> The columns are now nullable, but can you actually end up with a 
>>> null locale name in the expected field without manual DML on the 
>>> catalog, corruption or similar?  I think it should be a plain error 
>>> explaining the inconsistency rather then silently assuming there's 
>>> no version.  Note that at least
>>> pg_newlocale_from_collation() asserts that the specific libc/icu 
>>> field it's interested in isn't null.
>>
>> This is required because the default collations's fields are null now.
> 
> Yes I saw that, but that's a specific exception.  Detecting whether 
> it's the DEFAULT_COLLATION_OID or not and raise an error when a null 
> value isn't expected seems like it could be worthwhile.

I have fixed that as you suggest.

> So apart from the few details mentioned above I'm happy with this patch!

committed!




pg_database_iculocale_v1.diff
Description: pg_database_iculocale_v1.diff

Re: Logical replication timeout problem

[Masahiko Sawada]

On Thu, Mar 17, 2022 at 7:14 PM Amit Kapila  wrote:
>
> On Thu, Mar 17, 2022 at 12:27 PM Amit Kapila  wrote:
> >
> > On Wed, Mar 16, 2022 at 7:38 PM Masahiko Sawada  
> > wrote:
> > >
> > > After more thought, can we check only wal_sender_timeout without
> > > skip-count? That is, in WalSndUpdateProgress(), if we have received
> > > any reply from the subscriber in last (wal_sender_timeout / 2), we
> > > don't need to do anything in terms of keep-alive. If not, we do
> > > ProcessRepliesIfAny() (and probably WalSndCheckTimeOut()?) then
> > > WalSndKeepalivesIfNecessary(). That way, we can send keep-alive
> > > messages every (wal_sender_timeout / 2). And since we don't call them
> > > for every change, we would not need to worry about the overhead much.
> > >
> >
> > But won't that lead to a call to GetCurrentTimestamp() for each change
> > we skip? IIUC from previous replies that lead to a slight slowdown in
> > previous tests of Wang-San.
> >
> If the above is true then I think we can use a lower skip_count say 10
> along with a timeout mechanism to send keepalive message. This will
> help us to alleviate the overhead Wang-San has shown.

Using both sounds reasonable to me. I'd like to see how much the
overhead is alleviated by using skip_count 10 (or 100).

> BTW, I think there could be one other advantage of using
> ProcessRepliesIfAny() (as you are suggesting) is that it can help to
> release sync waiters if there are any. I feel that would be the case
> for the skip_empty_transactions patch [1] which uses
> WalSndUpdateProgress to send keepalive messages after skipping empty
> transactions.

+1

Regards,

[1] 
https://www.postgresql.org/message-id/OS3PR01MB6275DFFDAC7A59FA148931529E209%40OS3PR01MB6275.jpnprd01.prod.outlook.com

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: logical replication empty transactions

[Amit Kapila]

On Wed, Mar 16, 2022 at 12:33 PM Ajin Cherian  wrote:
>
> On Mon, Mar 7, 2022 at 11:44 PM Ajin Cherian  wrote:
> >
> > Fixed.
> >

Review comments/suggestions:
=
1. Isn't it sufficient to call pgoutput_send_begin from
maybe_send_schema as that is commonplace for all others and is always
the first message we send? If so, I think we can remove it from other
places?
2. Can we write some comments to explain why we don't skip streaming
or prepared empty transactions and some possible solutions (the
protocol change and additional subscription parameter as discussed
[1]) as discussed in this thread pgoutput.c?
3. Can we add a simple test for it in one of the existing test
files(say in 001_rep_changes.pl)?
4. I think we can drop the skip streaming patch as we can't do that for now.


-- 
With Regards,
Amit Kapila.

Re: Teach pg_receivewal to use lz4 compression

[Justin Pryzby]

On Sat, Feb 12, 2022 at 12:52:40PM +0900, Michael Paquier wrote:
> On Fri, Feb 11, 2022 at 10:07:49AM -0500, Robert Haas wrote:
> > Over in 
> > http://postgr.es/m/CA+TgmoYUDEJga2qV_XbAZ=pgebaosgfmzz6ac4_srwom_+u...@mail.gmail.com
> > I was noticing that CreateWalTarMethod doesn't support LZ4
> > compression. It would be nice if it did. I thought maybe the patch on
> > this thread would fix that, but I think maybe it doesn't, because it
> > looks like that's touching the WalDirectoryMethod part of that file,
> > rather than the WalTarMethod part. Is that correct?
> 
> Correct.  pg_receivewal only cares about the directory method, so this
> thread was limited to this part.  Yes, it would be nice to extend
> fully the tar method of walmethods.c to support LZ4, but I was not
> sure what needed to be done, and I am still not sure based on what has
> just been done as of 751b8d23.
> 
> > And, on a related note, Michael, do you plan to get something
> > committed here? 
> 
> Apart from f79962d, ba5 and 50e1441, I don't think that there was
> something left to do for this thread.  Perhaps I am missing something?

I think this should use 

+#include "lz4frame.h"

commit ba595d2322da095a1e6703171b3f1f2815cb
Author: Michael Paquier 
Date:   Fri Nov 5 11:33:25 2021 +0900

Add support for LZ4 compression in pg_receivewal

-- 
Justin

Re: Proposal: Support custom authentication methods using hooks

[Peter Eisentraut]

On 16.03.22 21:27, samay sharma wrote:
The only goal of this patch wasn't to enable support for Azure AD. 
That's just one client. Users might have a need to add or change auth 
methods in the future and providing that extensibility so we don't need 
to have core changes for each one of them would be useful. I know there 
isn't alignment on this yet, but if we'd like to move certain auth 
methods out of core into extensions, then this might provide a good 
framework for that.


Looking at the existing authentication methods

# METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
# "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".

how many of these could have been implemented using a plugin mechanism 
that was designed before the new method was considered?  Probably not 
many.  So I am fundamentally confused how this patch set can make such 
an ambitious claim.  Maybe the scope needs to be clarified first.  What 
kinds of authentication methods do you want to plug in?  What kinds of 
methods are out of scope?  What are examples of each one?

Re: Skipping logical replication transactions on subscriber side

[Masahiko Sawada]

On Thu, Mar 17, 2022 at 5:52 PM Amit Kapila  wrote:
>
> On Thu, Mar 17, 2022 at 12:39 PM osumi.takami...@fujitsu.com
>  wrote:
> >
> > On Thursday, March 17, 2022 3:04 PM Amit Kapila  
> > wrote:
> > > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada
> > >  wrote:
> > > >
> > > > I've attached an updated version patch.
> > > >
> > >
> > > The patch LGTM. I have made minor changes in comments and docs in the
> > > attached patch. Kindly let me know what you think of the attached?
> > Hi, thank you for the patch. Few minor comments.
> >
> >
> > (1) comment of maybe_start_skipping_changes
> >
> >
> > +   /*
> > +* Quick return if it's not requested to skip this transaction. This
> > +* function is called for every remote transaction and we assume 
> > that
> > +* skipping the transaction is not used often.
> > +*/
> >
> > I feel this comment should explain more about our intention and
> > what it confirms. In a case when user requests skip,
> > but it doesn't match the condition, we don't start
> > skipping changes, strictly speaking.
> >
> > From:
> > Quick return if it's not requested to skip this transaction.
> >
> > To:
> > Quick return if we can't ensure possible skiplsn is set
> > and it equals to the finish LSN of this transaction.
> >
>
> Hmm, the current comment seems more appropriate. What you are
> suggesting is almost writing the code in sentence form.
>
> >
> > (2) 029_on_error.pl
> >
> > +   my $contents = slurp_file($node_subscriber->logfile, $offset);
> > +   $contents =~
> > + qr/processing remote data for replication origin \"pg_\d+\" 
> > during "INSERT" for replication target relation "public.tbl" in transaction 
> > \d+ finishe$
> > + or die "could not get error-LSN";
> >
> > I think we shouldn't use a lot of new words.
> >
> > How about a change below  ?
> >
> > From:
> > could not get error-LSN
> > To:
> > failed to find expected error message that contains finish LSN for SKIP 
> > option
> >
> >
> > (3) apply_handle_commit_internal
> >
> ...
> >
> > I feel if we move those two functions at the end
> > of the apply_handle_commit and apply_handle_stream_commit,
> > then we will have more aligned codes and improve readability.
> >

I think we cannot just move them to the end of apply_handle_commit()
and apply_handle_stream_commit(). Because if we do that, we end up
missing updating replication_session_origin_lsn/timestamp when
clearing the subskiplsn if we're skipping a non-stream transaction.

Basically, the apply worker differently handles 2pc transactions and
non-2pc transactions; we always prepare even empty transactions
whereas we don't commit empty non-2pc transactions. So I think we
don’t have to handle both in the same way.

> I think the intention is to avoid duplicate code as we have a common
> function that gets called from both of those.

Yes.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/