Thanks for your review. I considered all of this along with your review
comments in another email prior to sending v7 in response to that other email a
few minutes ago.
> On Jan 28, 2020, at 7:17 AM, Robert Haas <robertmh...@gmail.com> wrote:
>
> On Mon, Jan 27, 2020 at 3:05 PM Mark Dilger
> <mark.dil...@enterprisedb.com> wrote:
>> I’m attaching a new patch set with these three changes including Mahendra’s
>> patch posted elsewhere on this thread.
>>
>> Since you’ve committed your 0004 and 0005 patches, this v6 patch set is now
>> based on a fresh copy of master.
>
> OK, so I think this is getting close.
>
> What is now 0001 manages to have four (4) conditionals on FRONTEND at
> the top of the file. This seems like at least one two many.
You are referencing this section, copied here from the patch:
> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #endif
>
> #include "common/jsonapi.h"
>
> #ifdef FRONTEND
> #include "common/logging.h"
> #endif
>
> #include "mb/pg_wchar.h"
>
> #ifndef FRONTEND
> #include "miscadmin.h"
> #endif
I merged these a bit. See v7-0001 for details.
> Also, the preprocessor police are on their way to your house now to
> arrest you for that first one. You need to write it like this:
>
> #define json_log_and_abort(...) \
> do { pg_log_fatal(__VA_ARGS__); exit(1); } while (0)
Yes, right, I had done that and somehow didn’t get it into the patch. I’ll
have coffee and donuts waiting.
> {
> - JsonLexContext *lex = palloc0(sizeof(JsonLexContext));
> + JsonLexContext *lex;
> +
> +#ifndef FRONTEND
> + lex = palloc0(sizeof(JsonLexContext));
> +#else
> + lex = (JsonLexContext*) malloc(sizeof(JsonLexContext));
> + memset(lex, 0, sizeof(JsonLexContext));
> +#endif
>
> Instead of this, how making no change at all here?
Yes, good point. I had split that into frontend vs backend because I was using
palloc0fast for the backend, which seems to me the preferred function when the
size is compile-time known, like it is here, and there is no palloc0fast in
fe_memutils.h for frontend use. I then changed back to palloc0 when I noticed
that pretty much nowhere else similar to this in the project uses palloc0fast.
I neglected to change back completely, which left what you are quoting.
Out of curiousity, why is palloc0fast not used in more places?
> - default:
> - elog(ERROR, "unexpected json parse state: %d", ctx);
> }
> +
> + /* Not reached */
> + json_log_and_abort("unexpected json parse state: %d", ctx);
>
> This, too, seems unnecessary.
This was in response to Mahendra’s report of a compiler warning, which I didn’t
get on my platform. The code in master changed a bit since v6 was written, so
v7 just goes with how the newer committed code does this.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
