From: | Brian Weaver <cmdrclueless(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Patch: incorrect array offset in backend replication tar header |
Date: | 2012-09-28 02:00:09 |
Message-ID: | CAAhXZGuEi9W3kEyZh0-pdnWw_hU1c3NrTvyQxrH2YM=N_4keGg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Sep 27, 2012 at 6:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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
Here's a very minimal fix then, perhaps it will be more palatable.
Even though I regret the effort I put into the first patch it's in my
employer's best interest that it's fixed. I'm obliged to try to
remediate the problem in something more acceptable to the community.
enjoy
--
/* insert witty comment here */
Attachment | Content-Type | Size |
---|---|---|
postgresql-tar.patch | application/octet-stream | 2.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2012-09-28 03:29:44 | setting per-database/role parameters checks them against wrong context |
Previous Message | Euler Taveira | 2012-09-28 00:08:47 | Re: Switching timeline over streaming replication |