Re: pg_rewind failure by file deletion in source server

On Mon, Jun 29, 2015 at 3:46 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 06/24/2015 09:43 AM, Michael Paquier wrote:
>>
>> Attached is a new set of patches. Except for the last ones that
>> addresses one issue of pg_rewind (symlink management when streaming
>> PGDATA), all the others introduce if_not_exists options for the
>> functions of genfile.c. The pg_rewind stuff could be more polished
>> though. Feel free to comment.
>
> I've committed the additional option to the functions in genfile.c (I
> renamed it to "missing_ok", for clarity), and the pg_rewind changes to use
> that option.
>
> I ended up refactoring the patch quite a bit, so if you could double-check
> what I committed to make sure I didn't break anything, that would be great.

Thanks, the new patch looks far better than what I did, I noticed a
couple of typos in the docs though:
- s/behaviour/behavior, "behavior" is American English spelling, and
it is the one used elsewhere as well, hence I guess that it makes
sense to our it.
- s/an non-existent/a non-existent
- pg_proc.h is still using if_not_exists as in my patch (you corrected
it to use missing_ok).
Those are fixed as 0001 in the attached set.

> I didn't commit the tablespace or symlink handling changes yet, will review
> those separately.

Thanks. Attached are rebased versions that take into account your
previous changes as well (like the variable renaming, wrapper function
usage, etc). I also added missing_ok to pg_readlink for consistency,
and rebased the fix of pg_rewind for soft links with 0005. Note that
0005 does not use pg_tablespace_location(), still the patch series
include an implementation of missing_ok for it. Feel free to use it if
necessary.

> I also didn't commit the new regression test yet. It would indeed be nice to
> have one, but I think it was a few bricks shy of a load. It should work in a
> freshly initdb'd system, but not necessarily on an existing installation.
> First, it relied on the fact that postgresql.conf.auto exists, but a DBA
> might remove that if he wants to make sure the feature is not used.
> Secondly, it relied on the fact that pg_twophase is empty, but there is no
> guarantee of that either. Third, the error messages included in the expected
> output, e.g "No such file or directory", depend on the operating system and
> locale. And finally, it'd be nice to test more things, in particular the
> behaviour of different offsets and lengths to pg_read_binary_file(),
> although an incomplete test would be better than no test at all.

Portability is going to really reduce the test range, the only things
that we could test are:
- NULL results returned when missing_ok = true (with a dummy file
name/link/directory) as missing_ok = false would choke depending on
the platform as you mentioned.
- Ensure that those functions called by users without superuser rights
fail properly.
- Path format errors for each function, like that:
=# select pg_ls_dir('..');
ERROR: 42501: path must be in or below the current directory
LOCATION: convert_and_check_filename, genfile.c:78
For tests on pg_read_binary_file, do you think that there is one file
of PGDATA that we could use for scanning? I cannot think of one on the
top of my mind now (postgresql.conf or any configuration files could
be overridden by the user so they are out of scope, PG_VERSION is an
additional maintenance burden). Well, I think that those things are
still worth testing in any case and I think that you think so as well.
If you are fine with that I could wrap up a patch that's better than
nothing done for sure. Thoughts?

Now we could have a TAP test for this stuff, where we could have a
custom PGDATA with some dummy files that we know will be empty for
example, still it seems like an overkill to me for this purpose,
initdb is rather costly in this facility.. And on small machines.
Regards,
--
Michael