Re: deparsing utility commands

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> Thanks. I have adjusted the code to use ObjectAddress in all functions
> called by ProcessUtilitySlow; the patch is a bit tedious but seems
> pretty reasonable to me. As I said, there is quite a lot of churn.

Thanks for doing this!

> Also attached are some patches to make some commands return info about
> a secondary object regarding the execution, which can be used to know
> more about what's being executed:
>
> 1) ALTER DOMAIN ADD CONSTRAINT returns (in addition to the address of
> the doamin) the address of the new constraint.
>
> 2) ALTER OBJECT SET SCHEMA returns (in addition to the address of the
> object) the address of the old schema.
>
> 3) ALTER EXTENSION ADD/DROP OBJECT returns (in addition to the address
> of the extension) the address of the added/dropped object.
>
>
> Also attached is the part of these patches that have ALTER TABLE return
> the AttrNumber and OID of the affected object.
>
> I think this is all mostly uninteresting, but thought I'd throw it out
> for public review before pushing, just in case. I have fixed many
> issues in the rest of the branch meanwhile, and it's looking much better
> IMO, but much work remains there and will leave them for later.

Glad to hear that things are progressing well!

> Subject: [PATCH 1/7] Have RENAME routines return ObjectAddress rather than OID
[...]

> /*
> * renameatt - changes the name of a attribute in a relation
> + *
> + * The returned ObjectAddress is that of the pg_class address of the column.
> */
> -Oid
> +ObjectAddress
> renameatt(RenameStmt *stmt)

This comment didn't really come across sensibly to me. I'd suggest
instead:

The ObjectAddress returned is that of the column which was renamed.

Or something along those lines instead?

The rest of this patch looked fine to me.

> Subject: [PATCH 2/7] deparse/core: have COMMENT return an ObjectAddress, not OID
[...]

Looked fine to me.

> Subject: [PATCH 3/7] deparse/core: have ALTER TABLE return table OID and other data
[...]

> @@ -1844,7 +1844,7 @@ heap_drop_with_catalog(Oid relid)
> /*
> * Store a default expression for column attnum of relation rel.
> */
> -void
> +Oid
> StoreAttrDefault(Relation rel, AttrNumber attnum,
> Node *expr, bool is_internal)
> {
> @@ -1949,6 +1949,8 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
> */
> InvokeObjectPostCreateHookArg(AttrDefaultRelationId,
> RelationGetRelid(rel), attnum, is_internal);
> +
> + return attrdefOid;
> }

Why isn't this returning an ObjectAddress too? It even builds one up
for you in defobject. Also, the comment should be updated to reflect
that it's now returning something.

> /*
> @@ -1956,8 +1958,10 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
> *
> * Caller is responsible for updating the count of constraints
> * in the pg_class entry for the relation.
> + *
> + * The OID of the new constraint is returned.
> */
> -static void
> +static Oid
> StoreRelCheck(Relation rel, char *ccname, Node *expr,
> bool is_validated, bool is_local, int inhcount,
> bool is_no_inherit, bool is_internal)
> @@ -1967,6 +1971,7 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
> List *varList;
> int keycount;
> int16 *attNos;
> + Oid constrOid;
>
> /*
> * Flatten expression to string form for storage.
> @@ -2018,7 +2023,7 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
> /*
> * Create the Check Constraint
> */
> - CreateConstraintEntry(ccname, /* Constraint Name */
> + constrOid = CreateConstraintEntry(ccname, /* Constraint Name */
> RelationGetNamespace(rel), /* namespace */
> CONSTRAINT_CHECK, /* Constraint Type */
> false, /* Is Deferrable */
> @@ -2049,11 +2054,15 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
>
> pfree(ccbin);
> pfree(ccsrc);
> +
> + return constrOid;
> }

Ditto here? CreateConstraintEntry would have to be updated to return
the ObjectAddress that it builds up, but there aren't all that many
callers of it..

> /*
> * Store defaults and constraints (passed as a list of CookedConstraint).
> *
> + * Each CookedConstraint struct is modified to store the new catalog tuple OID.
> + *
> * NOTE: only pre-cooked expressions will be passed this way, which is to
> * say expressions inherited from an existing relation. Newly parsed
> * expressions can be added later, by direct calls to StoreAttrDefault
> @@ -2065,7 +2074,7 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal)
> int numchecks = 0;
> ListCell *lc;
>
> - if (!cooked_constraints)
> + if (list_length(cooked_constraints) == 0)
> return; /* nothing to do */

I don't see any cases of 'list_length() == 0' in our code base. The
convention is (overwhelmingly, it seems): if (cooked_constraints == NIL)

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index f85ed93..5773298 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -1111,7 +1111,8 @@ index_create(Relation heapRelation,
> /*
> * index_constraint_create
> *
> - * Set up a constraint associated with an index
> + * Set up a constraint associated with an index. Return the new constraint's
> + * OID.
> *
> * heapRelation: table owning the index (must be suitably locked by caller)
> * indexRelationId: OID of the index
> @@ -1128,7 +1129,7 @@ index_create(Relation heapRelation,
> * allow_system_table_mods: allow table to be a system catalog
> * is_internal: index is constructed due to internal process
> */
> -void
> +Oid
> index_constraint_create(Relation heapRelation,
> Oid indexRelationId,
> IndexInfo *indexInfo,
> @@ -1316,6 +1317,8 @@ index_constraint_create(Relation heapRelation,
> heap_freetuple(indexTuple);
> heap_close(pg_index, RowExclusiveLock);
> }
> +
> + return conOid;
> }

Ditto here, it could return 'referenced' instead (which doesn't seem
like a very good name for that variable, if you ask me.. that's kind of
a side-note, but 'referenced' to me makes it sound like it's the *other
relation*, but the ObjectAddress constructed is clearly that of a
constraint and not a table).

> @@ -3410,78 +3412,95 @@ static void
> ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
> AlterTableCmd *cmd, LOCKMODE lockmode)
> {

Looking through this, I think I realize why you might have decided to
use the simpler "just the Oid" in many cases.. However, what that means
is that through this function, you have to know the subtype to know what
the Oid actually represents or if colno is supposed to be set or not.
Using a single ObjectAddress for all of these would mean that you know
what class the object is and that informs if you need a sub-id or not or
if the Oid should be set or not (obviously, lots of the below switch
paths end up setting a colno without newoid being set..).

I certainly understand that we don't want to transform Oid ->
ObjectAddress across the entire code base, but at these higher level
functions which are working with lots of different object classes, some
of which could be just class+Oid and some of which are class+Oid+sub-id,
I'm thinking we'd be better off using ObjectAddress.

Apologies for generating lots of work. :(

> case AT_ReplicaIdentity:
> - ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def, lockmode);
> + ATExecReplicaIdentity(rel, (ReplicaIdentityStmt *) cmd->def,
> + lockmode);

This is just a whitespace change..? I don't think I disagree with it,
but would prefer to not have just-whitespace changes in the patch if we
can avoid it, makes me think I'm missing something. :)

> -static void
> +static AttrNumber
> ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
> {
> HeapTuple tuple;
> @@ -5137,18 +5161,22 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
>
> /* keep the system catalog indexes current */
> CatalogUpdateIndexes(attr_rel, tuple);
> +
> + /* XXX should we return InvalidAttrNumber if there was no change? */

If we need that information to be returned, I think I'd rather do it in
another way rather than returning InvalidAttrNumber (or
InvalidObjectAddress) as a special value that means "did nothing".

> Subject: [PATCH 4/7] deparse/core: return ObjAddress rather than Oid
[...]

> @@ -1004,6 +1004,10 @@ AddNewRelationType(const char *typeName,
> * use_user_acl: TRUE if should look for user-defined default permissions;
> * if FALSE, relacl is always set NULL
> * allow_system_table_mods: TRUE to allow creation in system namespaces
> + * is_internal: is this a system-generated catalog?

Shouldn't adding the comment for is_internal be done independently?
It's clearly missing in master and fixing that hasn't got anything to do
with the other changes happening here.

This patch did make me think that there's quite a few places which could
use ObjectAddressSet() that aren't (even after this patch). I don't
think we need to stress over that, but it might be nice to mark that
down as a TODO for someone to go through all the places where we
construct an ObjectAddress and update them to use the new macro.

> @@ -613,10 +614,14 @@ DefineIndex(Oid relationId,
> stmt->concurrent, !check_rights,
> stmt->if_not_exists);
>
> + address.classId = RelationRelationId;
> + address.objectId = indexRelationId;
> + address.objectSubId = 0;

Like this bit of code (wrt using the macro)?

> @@ -772,7 +775,7 @@ DefineOpFamily(CreateOpFamilyStmt *stmt)
> * other commands called ALTER OPERATOR FAMILY exist, but go through
> * different code paths.
> */
> -Oid
> +void
> AlterOpFamily(AlterOpFamilyStmt *stmt)
> {
> Oid amoid, /* our AM's oid */
> @@ -826,8 +829,6 @@ AlterOpFamily(AlterOpFamilyStmt *stmt)
> AlterOpFamilyAdd(stmt->opfamilyname, amoid, opfamilyoid,
> maxOpNumber, maxProcNumber,
> stmt->items);
> -
> - return opfamilyoid;
> }

This feels a bit funny to me..? Why not construct and return an
ObjectAddress like the other Alter functions?

> @@ -690,13 +694,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
> AddRelationNewConstraints(rel, rawDefaults, stmt->constraints,
> true, true, false);
>
> + address.classId = RelationRelationId;
> + address.objectId = relationId;
> + address.objectSubId = 0;

macro?

> @@ -1366,7 +1367,6 @@ renametrig(RenameStmt *stmt)
> return address;
> }
>
> -
> /*
> * EnableDisableTrigger()
> *

whitespace change

> @@ -2249,19 +2253,25 @@ AlterDomainDefault(List *names, Node *defaultRaw)
>
> InvokeObjectPostAlterHook(TypeRelationId, domainoid, 0);
>
> + address.classId = TypeRelationId;
> + address.objectId = domainoid;
> + address.objectSubId = 0;

macro?

> @@ -2363,11 +2374,15 @@ AlterDomainNotNull(List *names, bool notNull)
>
> InvokeObjectPostAlterHook(TypeRelationId, domainoid, 0);
>
> + address.classId = TypeRelationId;
> + address.objectId = domainoid;
> + address.objectSubId = 0;

macro?

> @@ -2434,6 +2450,11 @@ AlterDomainDropConstraint(List *names, const char *constrName,
> found = true;
> }
> }
> +
> + address.classId = TypeRelationId;
> + address.objectId = domainoid;
> + address.objectSubId = 0;

macro?

> @@ -2471,6 +2492,7 @@ AlterDomainAddConstraint(List *names, Node *newConstraint)
> Form_pg_type typTup;
> Constraint *constr;
> char *ccbin;
> + ObjectAddress address;
>
> /* Make a TypeName so we can use standard type lookup machinery */
> typename = makeTypeNameFromNameList(names);
> @@ -2555,10 +2577,14 @@ AlterDomainAddConstraint(List *names, Node *newConstraint)
> if (!constr->skip_validation)
> validateDomainConstraint(domainoid, ccbin);
>
> + address.classId = TypeRelationId;
> + address.objectId = domainoid;
> + address.objectSubId = 0;

macro?

> @@ -2654,6 +2681,10 @@ AlterDomainValidateConstraint(List *names, char *constrName)
> InvokeObjectPostAlterHook(ConstraintRelationId,
> HeapTupleGetOid(copyTuple), 0);
>
> + address.classId = TypeRelationId;
> + address.objectId = domainoid;
> + address.objectSubId = 0;

macro?

> @@ -208,16 +209,20 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
> /* OK, let's do it. */
> AlterTableInternal(viewOid, atcmds, true);
>
> + address.classId = RelationRelationId;
> + address.objectId = viewOid;
> + address.objectSubId = 0;

macro?

Rest of this patched looked good to me.

> Subject: [PATCH 5/7] deparse/core: have ALTER EXTENSION ADD/DROP report ObjAddr of affected object
[...]
> /*
> * Execute ALTER EXTENSION ADD/DROP
> + *
> + * Return value is the address of the altered extension; objAddr, if not NULL,
> + * is set to the address of the added/dropped object.
> */

I would make those two sentences (or a bulletted list, perhaps). The
first sentence fragment and the second have really got nothing to do
with each other and can stand alone, so using a semi-colon seems a bit
off. Other places you've made it very clear with "Output Argument" or
similar, that'd work too.

Otherwise, this looked fine to me.

> Subject: [PATCH 6/7] Have ALTER/SET SCHEMA return the original schema's ObjectAddress
[...]

> @@ -393,26 +393,39 @@ ExecRenameStmt(RenameStmt *stmt)
> /*
> * Executes an ALTER OBJECT / SET SCHEMA statement. Based on the object
> * type, the function appropriate to that type is executed.
> + *
> + * Return value is that of the altered object. oldSchemaAddr, if not NULL, is
> + * set to the object address of the original schema.
> */

This is better, but would probably be even better if it was on a seperate
line or a bulletted list or something.

> --- a/src/backend/commands/extension.c
> +++ b/src/backend/commands/extension.c
> @@ -2402,7 +2402,7 @@ extension_config_remove(Oid extensionoid, Oid tableoid)
> * Execute ALTER EXTENSION SET SCHEMA
> */
> ObjectAddress
> -AlterExtensionNamespace(List *names, const char *newschema)
> +AlterExtensionNamespace(List *names, const char *newschema, Oid *oldschema)
> {
> char *extensionName;
> Oid extensionOid;

Output parameter is not an ObjectAddress for this one? The other case
where a schema was an output parameter, it was.. Feels a little odd.

Also, the function comment needs updating to reflect the new output
parameter.

> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 733b987..92099a2 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -11048,7 +11048,7 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
> * Execute ALTER TABLE SET SCHEMA
> */
> ObjectAddress
> -AlterTableNamespace(AlterObjectSchemaStmt *stmt)
> +AlterTableNamespace(AlterObjectSchemaStmt *stmt, Oid *oldschema)
> {
> Relation rel;
> Oid relid;

Ditto here.

>
> diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
> index e246719..4df7e64 100644
> --- a/src/backend/commands/typecmds.c
> +++ b/src/backend/commands/typecmds.c
> @@ -3540,11 +3540,13 @@ AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId,
> * Execute ALTER TYPE SET SCHEMA
> */
> ObjectAddress
> -AlterTypeNamespace(List *names, const char *newschema, ObjectType objecttype)
> +AlterTypeNamespace(List *names, const char *newschema, ObjectType objecttype,
> + Oid *oldschema)

And here.

The rest of this patch looked fine to me.

> Subject: [PATCH 7/7] deparse/core: have ALTER DOMAIN return ObjAddr of affected constraint
[...]

> @@ -2483,7 +2483,8 @@ AlterDomainDropConstraint(List *names, const char *constrName,
> * Implements the ALTER DOMAIN .. ADD CONSTRAINT statement.
> */
> ObjectAddress
> -AlterDomainAddConstraint(List *names, Node *newConstraint)
> +AlterDomainAddConstraint(List *names, Node *newConstraint,
> + ObjectAddress *constrAddr)
> {
> TypeName *typename;
> Oid domainoid;

Function header comment needs updating.

> @@ -2991,13 +2992,14 @@ checkDomainOwner(HeapTuple tup)
> static char *
> domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
> int typMod, Constraint *constr,
> - char *domainName)
> + char *domainName, ObjectAddress *constrAddr)

Function header comment needs updating.

Rest of this patch looked fine to me.

Thanks for all of this! Obviously, that was just a (well, not so) quick
review; I didn't try to compile or run it or anything.

Not sure if it's most helpful for me to continue to provide reviews or
if it'd be useful for me to help with actually making some of these
changes- please let me know your thoughts and I'll do my best to help.

Thanks again!

Stephen