Thread: pgsql: Fix pg_basebackup with in-place tablespaces.
Fix pg_basebackup with in-place tablespaces. Previously, pg_basebackup from a cluster that contained an 'in-place' tablespace, as introduced by commit 7170f215, would produce a harmless warning on Unix and fail completely on Windows. Reported-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/20220304.165449.1200020258723305904.horikyota.ntt%40gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/c6f2f01611d4f2c412e92eb7893f76fa590818e8 Modified Files -------------- src/backend/access/transam/xlog.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
On 3/14/22 19:31, Thomas Munro wrote:
> Fix pg_basebackup with in-place tablespaces.
>
> Previously, pg_basebackup from a cluster that contained an 'in-place'
> tablespace, as introduced by commit 7170f215, would produce a harmless
> warning on Unix and fail completely on Windows.
Perhaps I'm being picky, but seems like this logic should be wrapped in:
if (allow_in_place_tablespaces)
{
<...>
}
I worry about strange effects when this GUC is not enabled.
Regards,
-David
David Steele <david@pgmasters.net> writes:
> On 3/14/22 19:31, Thomas Munro wrote:
>> Fix pg_basebackup with in-place tablespaces.
> Perhaps I'm being picky, but seems like this logic should be wrapped in:
> if (allow_in_place_tablespaces)
> {
> <...>
> }
> I worry about strange effects when this GUC is not enabled.
What would happen if someone created an in-place tablespace and
then turned off the GUC?
regards, tom lane
On Wed, Mar 16, 2022 at 10:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Steele <david@pgmasters.net> writes:
> > On 3/14/22 19:31, Thomas Munro wrote:
> >> Fix pg_basebackup with in-place tablespaces.
>
> > Perhaps I'm being picky, but seems like this logic should be wrapped in:
> > if (allow_in_place_tablespaces)
> > {
> > <...>
> > }
> > I worry about strange effects when this GUC is not enabled.
>
> What would happen if someone created an in-place tablespace and
> then turned off the GUC?
Then it would break with unhelpful error messages. I suppose we could
error out explicitly, "in-place tablespace detected, but
allow_in_place_tablespaces not enabled". I'm not sure why we should
suddenly choose to enforce that *here* though.
At Wed, 16 Mar 2022 11:13:53 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in
> On Wed, Mar 16, 2022 at 10:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > David Steele <david@pgmasters.net> writes:
> > > On 3/14/22 19:31, Thomas Munro wrote:
> > >> Fix pg_basebackup with in-place tablespaces.
> >
> > > Perhaps I'm being picky, but seems like this logic should be wrapped in:
> > > if (allow_in_place_tablespaces)
> > > {
> > > <...>
> > > }
> > > I worry about strange effects when this GUC is not enabled.
> >
> > What would happen if someone created an in-place tablespace and
> > then turned off the GUC?
>
> Then it would break with unhelpful error messages. I suppose we could
> error out explicitly, "in-place tablespace detected, but
> allow_in_place_tablespaces not enabled". I'm not sure why we should
> suddenly choose to enforce that *here* though.
+1. The GUC is only to prevent non-developer users from accidentally
creating in-place tablespaces that is not officieally suported. We
have done nothing about in-place tablespaces other than the things
needed to perform regression test, and I think we won't do that in
future.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 3/15/22 23:42, Kyotaro Horiguchi wrote:
> At Wed, 16 Mar 2022 11:13:53 +1300, Thomas Munro <thomas.munro@gmail.com> wrote in
>> On Wed, Mar 16, 2022 at 10:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> David Steele <david@pgmasters.net> writes:
>>>> On 3/14/22 19:31, Thomas Munro wrote:
>>>>> Fix pg_basebackup with in-place tablespaces.
>>>
>>>> Perhaps I'm being picky, but seems like this logic should be wrapped in:
>>>> if (allow_in_place_tablespaces)
>>>> {
>>>> <...>
>>>> }
>>>> I worry about strange effects when this GUC is not enabled.
>>>
>>> What would happen if someone created an in-place tablespace and
>>> then turned off the GUC?
>>
>> Then it would break with unhelpful error messages. I suppose we could
>> error out explicitly, "in-place tablespace detected, but
>> allow_in_place_tablespaces not enabled". I'm not sure why we should
>> suddenly choose to enforce that *here* though.
>
> +1. The GUC is only to prevent non-developer users from accidentally
> creating in-place tablespaces that is not officieally suported. We
> have done nothing about in-place tablespaces other than the things
> needed to perform regression test, and I think we won't do that in
> future.
Sure, but there is a behavioral change whether the GUC is enabled or
not. Before, if there was clutter in pg_tblspc there would at least be a
warning in the log. Now that logging does not happen.
It seems that the warning at line 8313 is essentially dead code now. I
don't expect test code to have an impact on production systems, even if
the effect is minor.
I'm less worried about what happens when the flag is flipped on then off
because that's not likely to happen in production.
Regards,
-David
On Thu, Mar 17, 2022 at 3:29 AM David Steele <david@pgmasters.net> wrote:
> Sure, but there is a behavioral change whether the GUC is enabled or
> not. Before, if there was clutter in pg_tblspc there would at least be a
> warning in the log. Now that logging does not happen.
If there's clutter that doesn't look like an OID, we already ignore it
silently here:
/* Skip anything that doesn't look like a tablespace */
if (strspn(de->d_name, "0123456789") != strlen(de->d_name))
continue;
> It seems that the warning at line 8313 is essentially dead code now. I
> don't expect test code to have an impact on production systems, even if
> the effect is minor.
It's not dead, it's how we'd report something like EACCES or EIO. Why
we only warn and press on, I'm not sure. (I'm also looking into why
we ignore OS errors for pgwin32_is_junction everywhere.)
> I'm less worried about what happens when the flag is flipped on then off
> because that's not likely to happen in production.
So, concretely, if there is a non-symlink with a name that looks like
an OID under pg_tblspc, previously we'd barf (or apparently press on
with an empty pathname on Windows, which might indicate a lack of
error checking somewhere). Given the policy of quietly ignoring
anything else in the directory, is it really this code's job to sanity
check the cluster layout? Hmm, I guess we could fix the problem on
Windows in a different way so that it behaves like Unix, and then
revert this. You'd get an ignorable not-a-symlink warning as before
(and now it'd work on Windows) when backing up in-place tablespaces,
but I'm not sure it's really an improvement.
On 3/16/22 13:33, Thomas Munro wrote: > >> It seems that the warning at line 8313 is essentially dead code now. I >> don't expect test code to have an impact on production systems, even if >> the effect is minor. > > It's not dead, it's how we'd report something like EACCES or EIO. Why > we only warn and press on, I'm not sure. (I'm also looking into why > we ignore OS errors for pgwin32_is_junction everywhere.) I'm also wondering why this is only a warning. >> I'm less worried about what happens when the flag is flipped on then off >> because that's not likely to happen in production. > > So, concretely, if there is a non-symlink with a name that looks like > an OID under pg_tblspc, previously we'd barf (or apparently press on > with an empty pathname on Windows, which might indicate a lack of > error checking somewhere). Given the policy of quietly ignoring > anything else in the directory, is it really this code's job to sanity > check the cluster layout? Hmm, I guess we could fix the problem on > Windows in a different way so that it behaves like Unix, and then > revert this. You'd get an ignorable not-a-symlink warning as before > (and now it'd work on Windows) when backing up in-place tablespaces, > but I'm not sure it's really an improvement. I'm not going to press on this, but FWIW this solution sounds better to me. Either way it is a behavioral change. Windows used to error in this case and now it does not, Unix used to warn in this case and now it does not. For my 2C I'd rather this was a hard error because that makes life a lot easier down the line. But, that's certainly not the responsibility of this patch. Regards, -David