Re: SQL/MED - file_fdw

On Mon, Jan 17, 2011 at 06:33:08PM -0600, Kevin Grittner wrote:
> Itagaki Takahiro wrote:
> > Shigeru HANADA wrote:
>
> >> Attached patch would avoid this leak by adding per-copy context to
> >> CopyState. This would be overkill, and ResetCopyFrom() might be
> >> reasonable though.
> >
> > Good catch. I merged your fix into the attached patch.
>
> Review for CF:
...
> I tried to read through the code to look for problems, but there are
> so many structures and techniques involved that I haven't had to deal
> with yet, that it would take me days to get up to speed enough to
> desk check this adequately. Since this API is a prerequisite for
> other patches, and already being used by them, I figured I should do
> what I could quickly and then worry about how to cover that.

I've studied the patch from this angle. My two substantive questions concern
CopyFromErrorCallback() error strings and the use of the per-tuple memory
context; see below. The rest either entail trivial fixes, or they do not
indicate anything that clearly ought to change.

The patch mostly moves code around; it's a bit difficult to grasp what's really
changing by looking at the diff (not a criticism of the approach -- I see no way
to avoid that). The final code structure is at least as easy to follow as the
structure it replaces. Here is a summary of the changes:

file_fdw wishes to borrow the COPY FROM code for parsing structured text and
building tuples fitting a given relation. file_fdw must bypass the parts that
insert those tuples. Until now, copy.c has exposed a single API, DoCopy().
To meet file_fdw's needs, the patch adds four new APIs for use as a set:

BeginCopyFrom() - once-per-COPY initialization and validation
NextCopyFrom() - call N times to get N values/null arrays
EndCopyFrom() - once-per-COPY cleanup
CopyFromErrorCallback() - for caller use in ErrorContextCallback.callback

Implementing these entails breaking apart the existing code structure. For
one, the patch separates from DoCopy the once-per-COPY-statement code, both
initialization and cleanup. Secondly, it separates the CopyFrom code for
assembling a tuple based on COPY input from the code that actually stores said
tuples in the target relation. To avoid duplicating code, the patch then
calls these new functions from DoCopy and CopyFrom. To further avoid
duplicating code and to retain symmetry, it refactors COPY TO setup and
teardown along similar lines. We have four new static functions:

BeginCopyTo() - parallel to BeginCopyFrom(), for DoCopy() alone
BeginCopy() - shared bits between BeginCopyTo() and BeginCopyFrom()
EndCopyTo() - parallel to EndCopyFrom(), for DoCopy() alone
EndCopy() - shared bits between EndCopyTo() and EndCopyFrom()

Most "parse analysis"-type bits of DoCopy() move to BeginCopy(). Several
checks remain in DoCopy(): superuser for reading a server-local file,
permission on the relation, and a read-only transaction check. The first
stays where it does because a superuser may define a file_fdw foreign table
and then grant access to others. The other two remain in DoCopy() because the
new API uses the relation as a template without reading or writing it.

The catalog work (looking up defaults, I/O functions, etc) in CopyFrom() moves
to BeginCopyFrom(), and applicable local variables become CopyState members.

copy_in_error_callback changes name to CopyFromErrorCallback() to better fit
with the rest of the new API. Its implementation does not change at all.

Since it's now possible for one SQL statement to call into the COPY machinery
an arbitrary number of times, the patch introduces a per-COPY memory context.

The patch implements added refactorings that make sense but are peripheral to
the API change at hand. CopyState.fe_copy is gone, now calculated locally in
DoCopyTo(). CopyState.processed is gone, with the row count now bubbling up
through return values. We now initialize the CopyState buffers for COPY FROM
only; they are unused in COPY TO. The purest of patches would defer all these,
but I don't object to them tagging along.

For COPY TO, the relkind checks move from DoCopyTo() to BeginCopyTo(). I'm
skeptical about this one. It's not required for correctness, and the relkind
checks for COPY FROM remain in CopyFrom().

file_fdw uses CopyFromErrorCallback() to give errors the proper context. The
function uses template strings like "COPY %s, line %d", where %s is the name of
the relation being copied. Presumably file_fdw and other features using this
API would wish to customize that error message prefix, and the relation name
might not be apropos at all. How about another argument to BeginCopyFrom,
specifying an error prefix to be stashed in the CopyState?

We could easily regret requiring a Relation in BeginCopyFrom(); another user may
wish to use a fabricated TupleDesc. Code paths in the new API use the Relation
for a TupleDesc, relhashoids, column defaults, and error message strings.
Removing the need for a full Relation is within reach. That being said, the API
definitely loses cleanliness if we cater to that possibility now. It's probably
best to keep the API as the patch has it, and let the hypothetical future use
case change it.

There were some concerns upthread that exposing more of copy.c would attract use
in third party FDWs, hamstringing future efforts to improve the implementation
for the COPY command itself. On review, FDW modules are not likely users in
general. file_fdw wants this because it parses structured text files, not
because it's an FDW. Either way, the API also seems nicely-chosen to minimize
the exposure of internals.

I have not tested the patched code in any detail, because Kevin's review covered
that angle quite thoroughly. I have no cause to suspect that the patch will
change user-visible function behavior, aside from the spurious error message
changes noted below. It looks unlikely to affect performance, but that's
probably worth a quick test.

> Since it doesn't appear to be intended to change any user-visible
> behavior, I don't see any need for docs or changes to the regression
> tests. I didn't see any portability problems. It seemed a little
> light on comments to me, but perhaps that's because of my ignorance
> of some of the techniques being used -- maybe things are obvious
> enough to anyone who would be mucking about in copy.c.

In some places, the new code does not add comments despite the surrounding code
having many comments. However, some existing comments went a bit too far
(/* Place tuple in tuple slot */, /* Reset the per-tuple exprcontext */).
The degree of commenting is mostly good, but I've noted a few places below.

The header comment for BeginCopyFrom should probably explain the arguments a bit
more; it assumes familiarity with the COPY implementation. How about this:

/*
* Setup to read tuples from a file for COPY FROM.
*
* 'rel': used as a template for the tuples
* 'filename': name of server-local file to read
* 'attnamelist': List of char *, columns to include. NIL selects all cols.
* 'options': List of DefElem. See copy_opt_item in gram.y for selections.
*
* Returns a CopyState, to be passed to NextCopyFrom().
*/

Thanks,
nm

Comments on specific hunks:

> *** a/src/backend/commands/copy.c
> --- b/src/backend/commands/copy.c

> ***************
> *** 167,181 **** typedef struct CopyStateData
> char *raw_buf;
> int raw_buf_index; /* next byte to process */
> int raw_buf_len; /* total # of bytes stored */
> } CopyStateData;
>
> - typedef CopyStateData *CopyState;
> -
> /* DestReceiver for COPY (SELECT) TO */
> typedef struct
> {
> DestReceiver pub; /* publicly-known function pointers */
> CopyState cstate; /* CopyStateData for the command */
> } DR_copy;
>
>
> --- 170,197 ----
> char *raw_buf;
> int raw_buf_index; /* next byte to process */
> int raw_buf_len; /* total # of bytes stored */
> +
> + /*
> + * The definition of input functions and default expressions are stored
> + * in these variables.
> + */
> + EState *estate;
> + AttrNumber num_defaults;
> + bool file_has_oids;
> + FmgrInfo oid_in_function;
> + Oid oid_typioparam;
> + FmgrInfo *in_functions;
> + Oid *typioparams;
> + int *defmap;
> + ExprState **defexprs; /* array of default att expressions */
> } CopyStateData;

The leading comment only applies to a few of those fields. Each field needs its
own comment, instead.

> ***************
> *** 1314,1339 **** DoCopyTo(CopyState cstate)
> }
> PG_END_TRY();
>
> ! if (!pipe)
> {
> ! if (FreeFile(cstate->copy_file))
> ! ereport(ERROR,
> ! (errcode_for_file_access(),
> ! errmsg("could not write to file \"%s\": %m",
> ! cstate->filename)));
> }
> }
>
> /*
> * Copy from relation or query TO file.
> */
> ! static void
> CopyTo(CopyState cstate)
> {
> TupleDesc tupDesc;
> int num_phys_attrs;
> Form_pg_attribute *attr;
> ListCell *cur;
>
> if (cstate->rel)
> tupDesc = RelationGetDescr(cstate->rel);
> --- 1396,1442 ----
> }
> PG_END_TRY();
>
> ! return processed;
> ! }
> !
> ! /*
> ! * Clean up storage and release resources for COPY TO.
> ! */
> ! static void
> ! EndCopyTo(CopyState cstate)
> ! {
> ! if (cstate->filename != NULL && FreeFile(cstate->copy_file))
> ! ereport(ERROR,
> ! (errcode_for_file_access(),
> ! errmsg("could not close file \"%s\": %m",
> ! cstate->filename)));

The old error message was "could not write to file \"%s\": %m". This might be a
good change, but it belongs in a separate patch.

> !
> ! /*
> ! * Close the relation or query. We can release the AccessShareLock we got.
> ! */

Separated from its former location, this comment is not applicable.

> ***************
> *** 1826,1832 **** CopyFrom(CopyState cstate)
> slot = ExecInitExtraTupleSlot(estate);
> ExecSetSlotDescriptor(slot, tupDesc);
>
> ! econtext = GetPerTupleExprContext(estate);
>
> /*
> * Pick up the required catalog information for each attribute in the
> --- 1889,2070 ----
> slot = ExecInitExtraTupleSlot(estate);
> ExecSetSlotDescriptor(slot, tupDesc);
>
> ! /* Prepare to catch AFTER triggers. */
> ! AfterTriggerBeginQuery();
> !
> ! /*
> ! * Check BEFORE STATEMENT insertion triggers. It's debateable whether we
> ! * should do this for COPY, since it's not really an "INSERT" statement as
> ! * such. However, executing these triggers maintains consistency with the
> ! * EACH ROW triggers that we already fire on COPY.
> ! */
> ! ExecBSInsertTriggers(estate, resultRelInfo);
> !
> ! values = (Datum *) palloc(tupDesc->natts * sizeof(Datum));
> ! nulls = (bool *) palloc(tupDesc->natts * sizeof(bool));
> !
> ! bistate = GetBulkInsertState();
> !
> ! /* Set up callback to identify error line number */
> ! errcontext.callback = CopyFromErrorCallback;
> ! errcontext.arg = (void *) cstate;
> ! errcontext.previous = error_context_stack;
> ! error_context_stack = &errcontext;
> !
> ! while (!done)
> ! {

The loop may as well be unconditional ...

> ! bool skip_tuple;
> ! Oid loaded_oid = InvalidOid;
> !
> ! CHECK_FOR_INTERRUPTS();
> !
> ! /* Reset the per-tuple exprcontext */
> ! ResetPerTupleExprContext(estate);
> !
> ! /* Switch into its memory context */
> ! MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

Shouldn't a switch to this context happen inside NextCopyFrom(), then again for
the calls to heap_form_tuple/HeapTupleSetOid? I found previous discussion on
this matter, but no conclusion.

> !
> ! done = !NextCopyFrom(cstate, values, nulls, &loaded_oid);
> ! if (done)
> ! break;

... since this is the only exit. No need to maintain "done".

> ! /*
> ! * Clean up storage and release resources for COPY FROM.
> ! */
> ! void
> ! EndCopyFrom(CopyState cstate)
> ! {
> ! FreeExecutorState(cstate->estate);
> !
> ! if (cstate->filename != NULL && FreeFile(cstate->copy_file))
> ! ereport(ERROR,
> ! (errcode_for_file_access(),
> ! errmsg("could not close file \"%s\": %m",
> ! cstate->filename)));

Likewise: this might be a good verbiage change, but it belongs in its own patch.