Re: Support logical replication of DDLs - Mailing list pgsql-hackers

On 2023-Feb-03, Peter Smith wrote:

> 1.
> (This is not really a review comment - more just an observation...)
> 
> This patch seemed mostly like an assortment of random changes that
> don't seem to have anything in common except that some *later* patches
> of this set are apparently going to want them.

That's true, but from a submitter perspective it is 1000x easier to do
it like this, and for a reviewer these changes are not really very
interesting.  By now, given the amount of review effort that needs to go
into this patch (just because it's 800kb of diff), it seems fairly clear
that we cannot get this patch in time for v16, so it doesn't seem
priority to get this point sorted out.  Personally, from a review point
of view, I would still prefer to have it this way rather than each
change scattered in each individual patch that needs it, so let's not
get too worked out about it at this point.  Maybe if we can find some
use for some of these helpers in existing code that allow refactoring
while introducing these new functions, we can add them ahead of
everything else.

> 3. ExecuteGrantStmt
> 
> + /* Copy the grantor id needed for DDL deparsing of Grant */
> + istmt.grantor_uid = grantor;
> +
> 
> SUGGESTION (comment)
> Copy the grantor id to the parsetree, needed for DDL deparsing of Grant

Is istmt really "the parse tree" actually?  As I recall, it's a derived
struct that's created during execution of the grant/revoke command, so
modifying the comment like this would be a mistake.

> @@ -5922,7 +5922,7 @@ getObjectIdentityParts(const ObjectAddress *object,
>   transformType = format_type_be_qualified(transform->trftype);
>   transformLang = get_language_name(transform->trflang, false);
> 
> - appendStringInfo(&buffer, "for %s on language %s",
> + appendStringInfo(&buffer, "for %s language %s",
>   transformType,
>   transformLang);
> 
> There is no clue anywhere what this change was for.

We should get the objectIdentity changes ahead of everything else; I
think these can be qualified as bugs (though I would recommend not
backpatching them.)  I think there were two of these.

> 8.
> +/*
> + * Return the given object type as a string.
> + */
> +const char *
> +stringify_objtype(ObjectType objtype, bool isgrant)
> +{

> That 'is_grant' param seemed a bit hacky.
> 
> At least some comment should be given (maybe in the function header?)
> to explain why this boolean is modifying the return string.
> 
> Or maybe it is better to have another stringify_objtype_for_grant that
> just wraps this?

... I don't remember writing this code, but it's probably my fault (was
it 7 years ago now?).  Maybe we can find a different approach that
doesn't need yet another list of object types?  (If I did write it,) we
have a lot more infrastructure now that we had it back then, I think.
In any case it doesn't seem like a function called "stringify_objtype"
with this signature makes sense as an exported function, much less in
utility.c.


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"But static content is just dynamic content that isn't moving!"
                http://smylers.hates-software.com/2007/08/15/fe244d0c.html