Re: [PATCH] Make pg_basebackup configure and start standby [Review]

2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta:
> 2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:
>> 2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
>>> On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)at
>>> <mailto:zb(at)cybertec(dot)at>> wrote:
>>>
>>> 2013-01-02 01:24 keltezéssel, Tom Lane írta:
>>>
>>> Boszormenyi Zoltan <zb(at)cybertec(dot)at <mailto:zb(at)cybertec(dot)at>> writes:
>>>
>>> 2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
>>>
>>> That way we can get around the whole need for changing memory
>>> allocation across all the
>>> frontends, no? Like the attached.
>>>
>>> Sure it's simpler but then the consistent look of the code is lost.
>>> What about the other patch to unify pg_malloc and friends?
>>> Basically all client code boils down to
>>> fprintf(stderr, ...)
>>> in different disguise in their error reporting, so that patch can
>>> also be simplified but it seems that the atexit() - either explicitly
>>> or hidden behind InitPostgresFrontend() - cannot be avoided.
>>>
>>> Meh. I find it seriously wrongheaded that something as minor as an
>>> escape_quotes() function should get to dictate both malloc wrappers
>>> and error recovery handling throughout every program that might use it.
>>>
>>>
>>> Actually, the unification of pg_malloc and friends wasn't dictated
>>> by this little code, it was just that pg_basebackup doesn't provide
>>> a pg_malloc implementation (only pg_malloc0) that is used by
>>> initdb's escape_quotes() function. Then I noticed how wide these
>>> almost identical functions have spread into client apps already.
>>>
>>> I would say this unification patch is completely orthogonal to
>>> the patch in $SUBJECT. I will post it in a different thread if it's
>>> wanted at all. The extra atexit() handler is not needed if a simple
>>> fprintf(stderr, ...) error reporting is enough in all clients.
>>> As far as I saw, all clients do exactly this but some of them hide
>>> this behind #define's.
>>>
>>>
>>> Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's
>>> good or bad features.
>>>
>>> I like Magnus' version a lot better than that idea.
>>>
>>>
>>> OK, I will post the core patch building on his code.
>>>
>>>
>>> Thanks.
>>>
>>> A bigger issue that I notice with this code is that it's only correct in
>>> backend-safe encodings, as the comment mentions. If we're going to be
>>> putting it into frontend programs, how safe is that going to be?
>>>
>>> regards, tom lane
>>>
>>>
>>> The question in a different form is: does PostgreSQL support
>>> non-ASCII-safe encodings at all (or on the client side)? Forgive
>>> my ignorance and enlighten me: how many such encodings
>>> exist besides EBCDIC? Is UTF-16 non-ASCII-safe?
>>>
>>>
>>> We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.
>>>
>>> There are quite a few far-eastern encodings that aren't available as server
>>> encondings, and I believe it's all for this reason.
>>
>> I see, thanks.
>>
>>> That said, do we need to care *in this specific case*? We use it in initdb to parse
>>> config strings, I believe. And we'd use it to parse a conninfo string in
>>> pg_basebackup, correct?
>>
>> Correct.
>>
>>> Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii
>>> safe encodings, or rename it to indicate that it's limited in this?
>>
>> If you send a new patch with the function renamed accordingly, I will modify
>> my code to use it.
>
> Attached is the quotes-v2 patch, the function is renamed and
> the comment is modified plus the pg_basebackup v21 patch
> that uses this function.

Rebased after pgtar.h was added. The quotes.c comment was modified
even more so it doesn't say this function is used for SQL string literals.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/