Horton

Thread

Commits

  1. Fix server crash bug in 'server' backup target.

  2. Adjust server-side backup to depend on pg_write_server_files.

  1. Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-01-28T10:58:09Z

    Hi Hackers,

I just noticed that the new server-side base backup feature requires
superuser privileges (which is only documented in the pg_basebackup
manual, not in the streaming replication protocol specification).

Isn't this the kind of thing the pg_write_server_files role was created
for, so that it can be delegated to a non-superuser?

- ilmari

  2. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-01-28T13:44:09Z

    On Fri, Jan 28, 2022 at 5:58 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> I just noticed that the new server-side base backup feature requires
> superuser privileges (which is only documented in the pg_basebackup
> manual, not in the streaming replication protocol specification).
>
> Isn't this the kind of thing the pg_write_server_files role was created
> for, so that it can be delegated to a non-superuser?

That's a good idea. I didn't think of that. Would you like to propose a patch?

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

  3. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-01-28T15:28:20Z

    Robert Haas <robertmhaas@gmail.com> writes:

> On Fri, Jan 28, 2022 at 5:58 AM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>> I just noticed that the new server-side base backup feature requires
>> superuser privileges (which is only documented in the pg_basebackup
>> manual, not in the streaming replication protocol specification).
>>
>> Isn't this the kind of thing the pg_write_server_files role was created
>> for, so that it can be delegated to a non-superuser?
>
> That's a good idea. I didn't think of that. Would you like to propose a patch?

Sure, I'll try and whip something up over the weekend.

- ilmari

  4. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-01-28T17:15:59Z

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Robert Haas <robertmhaas@gmail.com> writes:
>
>> On Fri, Jan 28, 2022 at 5:58 AM Dagfinn Ilmari Mannsåker
>> <ilmari@ilmari.org> wrote:
>>> I just noticed that the new server-side base backup feature requires
>>> superuser privileges (which is only documented in the pg_basebackup
>>> manual, not in the streaming replication protocol specification).
>>>
>>> Isn't this the kind of thing the pg_write_server_files role was created
>>> for, so that it can be delegated to a non-superuser?
>>
>> That's a good idea. I didn't think of that. Would you like to propose a patch?
>
> Sure, I'll try and whip something up over the weekend.

Or now. Patch attached.

- ilmari

  5. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-01-28T17:33:24Z

    On Fri, Jan 28, 2022 at 12:16 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Or now. Patch attached.

LGTM. Committed.

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

  6. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-01-28T17:35:33Z

    On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
> LGTM. Committed.

Thanks!

- ilmari

  7. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-02T15:14:15Z

    On Fri, Jan 28, 2022 at 12:35 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
> > LGTM. Committed.
>
> Thanks!

It appears that neither of us actually tested that this works. For me,
it works when I test as a superuser, but if I test as a non-superuser
with or without pg_write_server_files, it crashes, because we end up
trying to do syscache lookups without a transaction environment. I
*think* that the attached is a sufficient fix; at least, it passes
simple testing.

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

  8. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-02-02T15:42:16Z

    Robert Haas <robertmhaas@gmail.com> writes:

> On Fri, Jan 28, 2022 at 12:35 PM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>> On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:
>> > LGTM. Committed.
>>
>> Thanks!
>
> It appears that neither of us actually tested that this works.

Oops!

> For me, it works when I test as a superuser, but if I test as a
> non-superuser with or without pg_write_server_files, it crashes,
> because we end up trying to do syscache lookups without a transaction
> environment. I *think* that the attached is a sufficient fix; at
> least, it passes simple testing.

Here's a follow-on patch that adds a test for non-superuser server-side
basebackup, which crashes without your patch and passes with it.

- ilmari

  9. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-02T15:57:28Z

    On Wed, Feb 2, 2022 at 10:42 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Here's a follow-on patch that adds a test for non-superuser server-side
> basebackup, which crashes without your patch and passes with it.

This seems like a good idea, but I'm not going to slip a change from
an exact test count to done_testing() into a commit on some other
topic...

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

  10. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-02T17:55:52Z

    Robert Haas <robertmhaas@gmail.com> writes:
> This seems like a good idea, but I'm not going to slip a change from
> an exact test count to done_testing() into a commit on some other
> topic...

Actually, it seemed that the consensus in the nearby thread [1]
was to start doing exactly that, rather than try to convert them
all in one big push.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/9D4FFB61-392B-4A2C-B7E4-911797B4AC14%40yesql.se

  11. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-02T18:44:18Z

    On Wed, Feb 2, 2022 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > This seems like a good idea, but I'm not going to slip a change from
> > an exact test count to done_testing() into a commit on some other
> > topic...
>
> Actually, it seemed that the consensus in the nearby thread [1]
> was to start doing exactly that, rather than try to convert them
> all in one big push.

Urk. Well, OK then.

Such an approach seems to me to have essentially nothing to recommend
it, but I just work here.

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

  12. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-02T18:46:39Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 2, 2022 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Actually, it seemed that the consensus in the nearby thread [1]
>> was to start doing exactly that, rather than try to convert them
>> all in one big push.

> Urk. Well, OK then.

> Such an approach seems to me to have essentially nothing to recommend
> it, but I just work here.

Well, if someone wants to step up and provide a patch that changes 'em
all at once, that'd be great.  But we've discussed this before and
nothing's happened.

			regards, tom lane

  13. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-02T18:50:17Z

    On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, if someone wants to step up and provide a patch that changes 'em
> all at once, that'd be great.  But we've discussed this before and
> nothing's happened.

I mean, I don't understand why it's even better. And I would go so far
as to say that if nobody can be bothered to do the work to convert
everything at once, it probably isn't better.

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

  14. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-02T18:58:15Z

    On Wed, Feb 2, 2022 at 1:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Well, if someone wants to step up and provide a patch that changes 'em
> > all at once, that'd be great.  But we've discussed this before and
> > nothing's happened.
>
> I mean, I don't understand why it's even better. And I would go so far
> as to say that if nobody can be bothered to do the work to convert
> everything at once, it probably isn't better.

And one thing that concretely stinks about is the progress reporting
you get while the tests are running:

t/010_pg_basebackup.pl ... 142/?

That's definitely less informative than 142/330 or whatever.

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

  15. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-02T19:36:11Z

    > On 2 Feb 2022, at 19:58, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> On Wed, Feb 2, 2022 at 1:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Feb 2, 2022 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, if someone wants to step up and provide a patch that changes 'em
>>> all at once, that'd be great.  But we've discussed this before and
>>> nothing's happened.
>> 
>> I mean, I don't understand why it's even better. And I would go so far
>> as to say that if nobody can be bothered to do the work to convert
>> everything at once, it probably isn't better.

I personally think it's better, so I went and did the work.  The attached is a
first pass over the tree to see what such a patch would look like.  This should
get a thread of it's own and not be hidden here but as it was discussed I piled
on for now.

> And one thing that concretely stinks about is the progress reporting
> you get while the tests are running:
> 
> t/010_pg_basebackup.pl ... 142/?
> 
> That's definitely less informative than 142/330 or whatever.

There is that.  That's less informative, but only when looking at the tests
while they are running.  There is no difference once the tests has finished so
CI runs etc are no less informative.  This however is something to consider.

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

  16. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-02T19:47:40Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 2 Feb 2022, at 19:58, Robert Haas <robertmhaas@gmail.com> wrote:
>> And one thing that concretely stinks about is the progress reporting
>> you get while the tests are running:
>> 
>> t/010_pg_basebackup.pl ... 142/?
>> 
>> That's definitely less informative than 142/330 or whatever.

> There is that.  That's less informative, but only when looking at the tests
> while they are running.  There is no difference once the tests has finished so
> CI runs etc are no less informative.  This however is something to consider.

TBH I don't see that that's worth much.  None of our tests run so long
that you'll be sitting there trying to estimate when it'll be done.
I'd rather have the benefit of not having to maintain the test counts.

			regards, tom lane

  17. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-02T22:43:31Z

    =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Here's a follow-on patch that adds a test for non-superuser server-side
> basebackup, which crashes without your patch and passes with it.

The Windows animals don't like this:

# Running: pg_basebackup --no-sync -cfast -U backupuser --target server:C:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_VGMM/backuponserver -X none
pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for user "backupuser"
not ok 108 - backup target server

#   Failed test 'backup target server'
#   at t/010_pg_basebackup.pl line 527.

Not sure whether we have a standard method to get around that.

			regards, tom lane

  18. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-02T22:52:56Z

    I wrote:
> The Windows animals don't like this:
> pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for user "backupuser"

> Not sure whether we have a standard method to get around that.

Ah, right, we do.  Looks like adding something like

auth_extra => [ '--create-role', 'backupuser' ]

to the $node->init call would do it, or you could mess with
invoking pg_regress --config-auth directly.

			regards, tom lane

  19. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-02-03T16:20:11Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> I wrote:
>> The Windows animals don't like this:
>> pg_basebackup: error: connection to server at "127.0.0.1", port 59539
>> failed: FATAL: SSPI authentication failed for user "backupuser"
>
>> Not sure whether we have a standard method to get around that.
>
> Ah, right, we do.  Looks like adding something like
>
> auth_extra => [ '--create-role', 'backupuser' ]
>
> to the $node->init call would do it, or you could mess with
> invoking pg_regress --config-auth directly.

This was enough incentive for me to set up Cirrus-CI for my fork on
GitHub, and the auth_extra approach in the attached patch fixed the
test:

https://cirrus-ci.com/task/6578617030279168?logs=test_bin#L21

- ilmari

  20. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-03T17:26:09Z

    On 2/2/22 17:52, Tom Lane wrote:
> I wrote:
>> The Windows animals don't like this:
>> pg_basebackup: error: connection to server at "127.0.0.1", port 59539 failed: FATAL:  SSPI authentication failed for user "backupuser"
>> Not sure whether we have a standard method to get around that.
> Ah, right, we do.  Looks like adding something like
>
> auth_extra => [ '--create-role', 'backupuser' ]
>
> to the $node->init call would do it, or you could mess with
> invoking pg_regress --config-auth directly.
>
> 			



I've fixed this using the auth_extra method, which avoids a reload.


cheers


andrew


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

  21. Re: Server-side base backup: why superuser, not pg_write_server_files?

    Robert Haas <robertmhaas@gmail.com> — 2022-02-03T18:10:57Z

    On Thu, Feb 3, 2022 at 12:26 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> I've fixed this using the auth_extra method, which avoids a reload.

Thank you much.

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