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

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: Magnus Hagander <magnus(at)hagander(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Make pg_basebackup configure and start standby [Review]
Date: 2013-01-02 10:54:07
Message-ID: 50E411CF.20802@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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/

Attachment Content-Type Size
quotes-v2.patch text/x-patch 3.9 KB
pg_basebackup-v21.patch text/x-patch 15.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Boszormenyi Zoltan 2013-01-02 12:59:50 [PATCH] Factor out pg_malloc and friends into port code
Previous Message Boszormenyi Zoltan 2013-01-02 09:37:21 Re: [PATCH] Make pg_basebackup configure and start standby [Review]