Re: making the backend's json parser work in frontend code - Mailing list pgsql-hackers

Thanks for your review.  I considered all of this along with your review comments in another email prior to sending v7
inresponse to that other email a few minutes ago. 

> On Jan 28, 2020, at 7:17 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Jan 27, 2020 at 3:05 PM Mark Dilger
> <mark.dilger@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
seemsto me the preferred function when the size is compile-time known, like it is here, and there is no palloc0fast in
fe_memutils.hfor frontend use.  I then changed back to palloc0 when I noticed that pretty much nowhere else similar to
thisin 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
changeda 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