Horton

Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Nikita Glukhov <n.gluhov@postgrespro.ru>, Andrew Dunstan <andrew@dunslane.net>, David Rowley <dgrowleyml@gmail.com>
Date: 2022-06-17T00:16:41Z
Lists: pgsql-hackers 
Hi,

On 2022-06-16 16:31:30 -0700, Andres Freund wrote:
> The EEOP_JSONEXPR stuff was added during 15 development in:
>
> commit 1a36bc9dba8eae90963a586d37b6457b32b2fed4
> Author: Andrew Dunstan <andrew@dunslane.net>
> Date:   2022-03-03 13:11:14 -0500
>
>     SQL/JSON query functions

I'm quite confused about part of the struct definition of this:

			struct JsonCoercionsState
			{
				struct JsonCoercionState
				{
					JsonCoercion *coercion; /* coercion expression */
					ExprState  *estate; /* coercion expression state */
				}			null,
							string,
				numeric    ,
							boolean,
							date,
							time,
							timetz,
							timestamp,
							timestamptz,
							composite;
			}			coercions;	/* states for coercion from SQL/JSON item
									 * types directly to the output type */

Why on earth do we have coercion state for all these different types? That
really adds up:

                struct {
                        JsonExpr * jsexpr;               /*    24     8 */
                        struct {
                                FmgrInfo func;           /*    32    48 */
                                /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
                                Oid typioparam;          /*    80     4 */
                        } input;                         /*    32    56 */

                        /* XXX last struct has 4 bytes of padding */

                        NullableDatum * formatted_expr;  /*    88     8 */
                        NullableDatum * res_expr;        /*    96     8 */
                        NullableDatum * coercion_expr;   /*   104     8 */
                        NullableDatum * pathspec;        /*   112     8 */
                        ExprState * result_expr;         /*   120     8 */
                        /* --- cacheline 2 boundary (128 bytes) --- */
                        ExprState * default_on_empty;    /*   128     8 */
                        ExprState * default_on_error;    /*   136     8 */
                        List *     args;                 /*   144     8 */
                        void *     cache;                /*   152     8 */
                        struct JsonCoercionsState coercions; /*   160   160 */
                } jsonexpr;                              /*    24   296 */

And why is FmgrInfo stored inline in the struct? Everything else just stores
pointers to FmgrInfo.


Now that I look at this: It's a *bad* idea to have subsidiary ExprState inside
an ExprState. Nearly always the correct thing to do is to build those
expressions. There's plenty memory and evaluation overhead in jumping to a
different expression. And I see no reason for doing it that way here?

This stuff doesn't look ready.

Greetings,

Andres Freund

Commits

  1. JSON_TABLE: Add support for NESTED paths and columns

  2. Add basic JSON_TABLE() functionality

  3. Add SQL/JSON query functions

  4. Add soft error handling to some expression nodes

  5. Adjust populate_record_field() to handle errors softly

  6. Refactor code used by jsonpath executor to fetch variables

  7. Add more SQL/JSON constructor functions

  8. SQL/JSON: support the IS JSON predicate

  9. SQL/JSON: add standard JSON constructor functions

  10. Add static assertion ensuring sizeof(ExprEvalStep) <= 64 bytes

  11. Remove size increase in ExprEvalStep caused by hashed saops

  12. pgstat: reduce timer overhead by leaving timer running.

  13. expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.

  14. SQL/JSON query functions

  15. Speedup ScalarArrayOpExpr evaluation