On Thu, Jun 8, 2023 at 10:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 6, 2023 at 4:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > Few assorted comments:
> > ===================
> >
>
> Few comments on 0008* patch:
> ==============================
> 1. After 0008*, deparse_CreateStmt(), forms dpcontext before forming
> identity whereas it is required only after it. It may not matter
> practically but it is better to do the work when it is required. Also,
> before 0008, it appears to be formed after identity, so not sure if
> the change in 0008 is intentional, if so, then please let me know the
> reason, and may be it is better to add a comment for the same.
>
Shifted dpcontext creation after identity.
> 2. Similarly, what is need to separately do insert_identity_object()
> in deparse_CreateStmt() instead of directly doing something like
> new_objtree_for_qualname() as we are doing in 0001 patch? For this, I
> guess you need objtype handling in new_jsonb_VA().
>
yes, for that we need object handling in new_jsonb_VA(), which is not
possible to do in this case as there is no standard format for the
jsonb object. As in this case, it is identity object which is of
format "identity": {"objname": "test1", "schemaname": "public"}, while
in some other case object could be say coltype which is of format :
"coltype": {"typmod": "", "typarray": false, "typename": "int4",
"schemaname": "pg_catalog"}. And we need to push each element of
these objects to output jsonbParseState as and when we read the
parse-tree instead of saving them to an intermediate structure and
then reading from that. This makes us construct json-objects outside
of new_jsonb_VA().
> 3.
> /*
> * It will be of array type for multi-columns table, so lets begin
> * an arrayobject. deparse_TableElems_ToJsonb() will add elements
> * to it.
> */
> pushJsonbValue(&state, WJB_BEGIN_ARRAY, NULL);
>
> deparse_TableElems_ToJsonb(state, relation, node->tableElts, dpcontext,
> false, /* not typed table */
> false); /* not composite */
> deparse_Constraints_ToJsonb(state, objectId);
>
> pushJsonbValue(&state, WJB_END_ARRAY, NULL);
>
> This part of the code is repeated in deparse_CreateStmt(). Can we move
> this to a separate function?
>
Common code shifted to another function insert_table_elements().
On similar line. added another new function table_elem_present() to
encapsulate 2 calls for the column and constraint into it.
> 4.
> * Note we ignore constraints in the parse node here; they are extracted from
> * system catalogs instead.
> */
>
> static void
> deparse_TableElems_ToJsonb(JsonbParseState *state, Relation relation,
>
> An extra empty line between the comments end and function appears unnecessary.
>
Modified.
> 5.
> + /* creat name and type elements for column */
>
> /creat/create
>
Modified.
Please find new set of patches addressing below:
a) Issue mentioned by Wang-san in [1],
b) Comments from Peter given in [2]
c) Comments from Amit given in the last 2 emails.
[1]:
https://www.postgresql.org/message-id/OS3PR01MB62750D43D4F7F075B33BD2609E52A%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPv9vPbUQc0fzrKmDkKOsS_bj-hup_E%2BsLHNEX%2B6F%2BSY5Q%40mail.gmail.com
Thank You Vignesh for handling (a), Ajin for handling (b), Shi-san and
Hou-san for contributing in (c).
The new changes are in patch 0001, 0002, 0005 and 0008.
thanks
Shveta