From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Zheng Li <zhengli10(at)gmail(dot)com>, li jie <ggysxcq(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, rajesh singarapu <rajesh(dot)rs0541(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Support logical replication of DDLs |
Date: | 2023-02-15 03:01:37 |
Message-ID: | OS0PR01MB57168C9B93D7B2DC4D72F8EF94A39@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
On Friday, February 10, 2023 7:07 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
>
> Comments on 0001 and 0002
> =======================
> 1.
> * CREATE COLLATION
> */
> ObjectAddress
> -DefineCollation(ParseState *pstate, List *names, List *parameters,
> bool if_not_exists)
> +DefineCollation(ParseState *pstate, List *names, List *parameters,
> + bool if_not_exists, ObjectAddress *from_existing_collid)
>
> I think it is better to expand function header comments to explain
> return values especially from_existing_collid.
Added.
> 2.
> +Form_pg_sequence_data
> +get_sequence_values(Oid sequenceId)
> +{
> + Buffer buf;
> + SeqTable elm;
> + Relation seqrel;
> + HeapTupleData seqtuple;
> + Form_pg_sequence_data seq;
> + Form_pg_sequence_data retSeq =
> palloc(sizeof(FormData_pg_sequence_data));
> +
> + /* Open and AccessShareLock sequence */
> + init_sequence(sequenceId, &elm, &seqrel);
>
> The comment above init_sequence() seems wrong to me. AFAICS, we
> acquire RowExclusiveLock in init_sequence() via
> lock_and_open_sequence(). Am, I missing anything?
Changed.
> 3.
> +get_sequence_values(Oid sequenceId)
> ...
> ...
> +
> + if (pg_class_aclcheck(sequenceId, GetUserId(),
> + ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>
> Why do we need to check UPDATE privilege just for reading the values?
I think it was mis-copied, removed.
>
> 4. I checked the callers of get_sequence_values and they just need
> 'last_val' but we still expose all values from Form_pg_sequence_data,
> not sure if that is required. In deparse_CreateSeqStmt(), we use it to
> append RESTART but the CREATE SEQUENCE syntax in docs doesn't have a
> RESTART clause, so I am confused as to why the patch appends the same.
> If it is really required then please add the comments for the same.
From the code, it seems CREATE SEQUENCE supports the RESTART keyword while the
document doesn’t mention it. I think I will start a separate thread to discuss
this.
>
> 5. In deparse_CreateSeqStmt() and deparse_ColumnIdentity(), we open
> SequenceRelationId, is that really required? Isn't locking the
> sequence relation sufficient as is done by get_sequence_values()?
> Also, I see that deparse_CreateSeqStmt() opens and locks the sequence
> whereas deparse_ColumnIdentity() doesn't do the same. Then, we unlock
> the relation in some cases but not in others (like get_sequence_values
> uses NoLock whereas others release the lock while closing the rel).
>
> 6. In get_sequence_values(), we check the permissions whereas callers
> just assume that it is done and don't check it while accessing the
> sequence. This makes the code a bit unclear.
>
> 7. After seeing the above inconsistencies, I am thinking will it be
> better to design get_sequence_values() such that it returns both
> sequence parameters and last_val in a structure and the callers use
> it. That would bit clean and avoid opening the relation multiple
> times.
Agreed. Refactored this as suggested.
>
> 8.
> +/*
> + * Return the given object type as a string.
> + * If isgrant is true, then this function is called
> + * while deparsing GRANT statement and some object
> + * names are replaced.
> + */
> +const char *
> +stringify_objtype(ObjectType objtype, bool isgrant)
>
> Have an empty line after the Return line. The line length appears too short.
Adjusted.
>
> 9. Similar to stringify_grant_objtype() and
> stringify_adefprivs_objtype(), shall we keep the list of all
> unsupported types in stringify_objtype()? That will help us to easily
> identify which objects are yet not supported.
Added.
>
> 10. In pg_get_ruledef_detailed(), the code to form a string for qual
> and action is mostly the same as what we have in make_ruledef(). I
> think we can extract the common code into a separate function to avoid
> missing the code updates at one of the places. I see that
> 'special_exprkind' is present in one place and not in other, it may be
> that over time, we have added new things to deparse_context which
> doesn't get updated in the patch. Also, I noticed that for
> CMD_NOTHING, the patch just ignores the action whereas the core code
> does append the definition. We should check whether such a difference
> is required and if so, then add comments for the same.
I extracted the command code into a separate function as suggested,
and fixed these inconsistences.
Here is the new version patch which addressed above comments.
I also fixed a bug for the deparsing of CREATE RULE that it didn't add
parentheses for rule action list.
And thanks for Vignesh to help addressing the comments.
Best Regards,
Hou zj
Attachment | Content-Type | Size |
---|---|---|
v69-0007-Document-DDL-replication-and-DDL-deparser.patch | application/octet-stream | 40.6 KB |
v69-0003-Support-DDL-deparse-of-the-rest-commands.patch | application/octet-stream | 200.9 KB |
v69-0002-Functions-to-deparse-Table-DDL-commands.patch | application/octet-stream | 131.3 KB |
v69-0001-Infrastructure-to-support-DDL-deparsing.patch | application/octet-stream | 40.8 KB |
v69-0004-Introduce-the-test_ddl_deparse_regress-test-modu.patch | application/octet-stream | 47.4 KB |
v69-0005-DDL-messaging-infrastructure-for-DDL-replication.patch | application/octet-stream | 41.6 KB |
v69-0006-Support-DDL-replication.patch | application/octet-stream | 212.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2023-02-15 04:00:26 | Re: Support logical replication of DDLs |
Previous Message | Adrian Klaver | 2023-02-15 01:35:48 | Re: Quoting issue from ODBC |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2023-02-15 03:25:13 | RE: Perform streaming logical transactions by background workers and parallel apply |
Previous Message | Kyotaro Horiguchi | 2023-02-15 02:59:44 | KeepLogSeg needs some fixes on behavior |