Re: Patch: incorrect array offset in backend replication tar header

Brian Weaver <cmdrclueless(at)gmail(dot)com> writes:
> OK, here is my attempt at patching and correcting the issue in this
> thread. I have done my best to test to ensure that hot standby,
> pg_basebackup, and pg_restore of older files work without issues. I
> think this might be a larger patch that expected, I took some
> liberties of trying to clean up a bit.

> For example the block size '512' was scattered throughout the code
> regarding the tar block size. I've replace instances of that with a
> defined constant TAR_BLOCK_SIZE. I've likewise created other constants
> and used them in place of raw numbers in what I hope makes the code a
> bit more readable.

That seems possibly reasonable ...

> I've also used functions like strncpy(), strnlen(), and the like in
> place of sprintf() where I could. Also instead of using sscanf() I
> used a custom octal conversion routine which has a hard limit on how
> many character it will process like strncpy() & strnlen().

... but I doubt that this really constitutes a readability improvement.
Or a portability improvement. strnlen for instance is not to be found
in Single Unix Spec v2 (http://pubs.opengroup.org/onlinepubs/007908799/)
which is what we usually take as our baseline assumption about which
system functions are available everywhere. By and large, I think the
more different system functions you rely on, the harder it is to read
your code, even if some unusual system function happens to exactly match
your needs in particular places. It also greatly increases the risk
of having portability problems, eg on Windows, or non-mainstream Unix
platforms.

But a larger point is that the immediate need is to fix bugs. Code
beautification is a separate activity and would be better submitted as
a separate patch. There is no way I'd consider applying most of this
patch to the back branches, for instance.

regards, tom lane