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

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

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

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

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

On Fri, 28 Jan 2022, at 17:33, Robert Haas wrote:

LGTM. Committed.

Thanks!

- ilmari

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

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

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

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]/messages/by-id/9D4FFB61-392B-4A2C-B7E4-911797B4AC14@yesql.se
was to start doing exactly that, rather than try to convert them
all in one big push.

regards, tom lane

[1]: /messages/by-id/9D4FFB61-392B-4A2C-B7E4-911797B4AC14@yesql.se

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

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

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

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

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/

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

=?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

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

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

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