Re: Portal->commandTag as an enum - Mailing list pgsql-hackers

On 2020-Mar-02, Mark Dilger wrote:

> I think it is more natural to change event trigger code to reason
> natively about CommandTags rather than continuing to reason about
> strings.  The conversion back-and-forth between the enum and the
> string representation serves no useful purpose that I can see.  But I
> understand if you are just trying to have the patch change fewer parts
> of the code, and if you feel more comfortable about reverting that
> part of the patch, as the committer, I think that's your prerogative.

Nah -- after reading it again, that would make no sense.  With the
change, we're forcing all writers of event trigger functions in C to
adapt their functions to the new API, but that's okay -- I don't expect
there will be many, and we're doing other things to the API anyway.

I pushed it now.

I made very small changes before pushing: notably, I removed the
InitializeQueryCompletion() call from standard_ProcessUtility; instead
its callers are supposed to do it.  Updated ProcessUtility's comment to
that effect.

Also, the affected plancache.c functions (CreateCachedPlan and
CreateOneShotCachedPlan) had not had their comments updated.  Previously
they received compile-time constants, but that was important because
they were strings.  No longer.

I noticed some other changes that could perhaps be made here, but didn't
do them; for instance, in pg_stat_statement we have a comparison to
CMDTAG_COPY that seems pointless; we could just acquire the value
always.  I left it alone for now but I think the change is without side
effects (notably so because most actual DML does not go through
ProcessUtility anyway).  Also, event_trigger.c could resolve command
strings to the tag enum earlier.

There's also a lot of nonsense in the pquery.c functions, such as this,

                /*
                 * Now fetch desired portion of results.
                 */
                nprocessed = PortalRunSelect(portal, true, count, dest);

                /*
                 * If the portal result contains a command tag and the caller
                 * gave us a pointer to store it, copy it and update the
                 * rowcount.
                 */
                if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
                {
                    CopyQueryCompletion(qc, &portal->qc);
                    qc->nprocessed = nprocessed;
                }

I think we could simplify that by passing the qc.

Similar consideration with DoCopy; instead of a 'uint64 nprocessed' we
could have a *qc to fill in and avoid this bit of silliness,

                DoCopy(pstate, (CopyStmt *) parsetree,
                       pstmt->stmt_location, pstmt->stmt_len,
                       &processed);
                if (qc)
                    SetQueryCompletion(qc, CMDTAG_COPY, processed);

I see no reason to have PortalRun() initialize the qc; ISTM that its
callers should do so.

And so on.

Nothing of that is critical.

Thanks for your patch,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services