Re: Bug with pg_basebackup and 'shared' tablespace

On Thu, Sep 14, 2017 at 2:17 AM, Pierre Ducroquet <p(dot)psql(at)pinaraf(dot)info> wrote:
> All my apologies for the schockingly long time with no answer on this topic.

No problem. That's the concept called life I suppose.

> I will do my best to help review some patches in the current CF.

Thanks for the potential help!

> Attached to this email is the new version of the patch, checked against HEAD
> and REL_10_STABLE, with the small change required by Michael (affect true/
> false to the boolean instead of 1/0) applied.

Thanks for the new version.

So I have been pondering about this patch, and allowing multiple
versions of Postgres to run in the same tablespace is mainly here for
upgrade purposes, so I don't think that we should encourage such
practices for performance reasons. There is as well
--tablespace-mapping which is here to cover a similar need and we know
that this solution works, at least people have spent time to make sure
it does.

Another problem that I have with this patch is that on HEAD,
permission checks are done before receiving any data. I think that
this is really saner to do any checks like that first, so as you can
know if the tablespace is available for write access before writing
any data to it. With this patch, you may finish by writing a bunch of
data to a tablespace path, but then fail on another tablespace because
of permission issues so the backup becomes useless after transferring
and writing potentially hundreds of gigabytes. This is no fun to
detect that potentially too late, and can impact the responsiveness of
instances to get backups and restore from them.

All in all, this patch gets a -1 from me in its current shape. If
Horiguchi-san or yourself want to process further with this patch, of
course feel free to do so, but I don't feel that we are actually
trying to solve a new problem here. I am switching the patch to
"waiting on author".

Here is a nitpick about the patch by the way:
+ if (firstfile && !basetablespace)
+ {
+ /*
+ * The first file in the tablespace is its
main folder, whose name can not be guessed (PG_MAJORVER_CATVER)
+ * So we must check here that this folder can
be created or is empty.
+ */
+ verify_dir_is_empty_or_create(filename,
&made_tablespace_dirs, &found_tablespace_dirs);
+ }
+ else if (mkdir(filename, S_IRWXU) != 0)
This patch has formatting problems. You should try to run a pgindent
run before submitting it. The code usually needs to fit within the
72-80 character window.
--
Michael