Re: COPY as a set returning function

David Fetter wrote:

> @@ -562,7 +563,6 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
> errmsg("could not read from COPY file: %m")));
> break;
> case COPY_OLD_FE:
> -
> /*
> * We cannot read more than minread bytes (which in practice is 1)
> * because old protocol doesn't have any clear way of separating

This change is pointless as it'd be undone by pgindent.

> + /*
> + * Function signature is:
> + * copy_srf( filename text default null,
> + * is_program boolean default false,
> + * format text default 'text',
> + * delimiter text default E'\t' in text mode, ',' in csv mode,
> + * null_string text default '\N',
> + * header boolean default false,
> + * quote text default '"' in csv mode only,
> + * escape text default null, -- defaults to whatever quote is
> + * encoding text default null
> + */

This comment would be mangled by pgindent -- please add an /*--- marker
to prevent it.

> + /* param 7: escape text default null, -- defaults to whatever quote is */
> + if (PG_ARGISNULL(7))
> + {
> + copy_state.escape = copy_state.quote;
> + }
> + else
> + {
> + if (copy_state.csv_mode)
> + {
> + copy_state.escape = TextDatumGetCString(PG_GETARG_TEXT_P(7));
> + if (strlen(copy_state.escape) != 1)
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("COPY escape must be a single one-byte character")));
> + }
> + }
> + else
> + {
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("COPY escape available only in CSV mode")));
> + }
> + }

I don't understand why do we have all these checks. Can't we just pass
the values to COPY and let it apply the checks? That way, when COPY is
updated to support multibyte escape chars (for example) we don't need to
touch this code. Together with removing the unneeded braces that would
make these stanzas about six lines long instead of fifteen.

> + tuple = heap_form_tuple(tupdesc,values,nulls);
> + //tuple = BuildTupleFromCStrings(attinmeta, field_strings);
> + tuplestore_puttuple(tupstore, tuple);

No need to form a tuple; use tuplestore_putvalues here.

> + }
> +
> + /* close "file" */
> + if (copy_state.is_program)
> + {
> + int pclose_rc;
> +
> + pclose_rc = ClosePipeStream(copy_state.copy_file);
> + if (pclose_rc == -1)
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not close pipe to external command: %m")));
> + else if (pclose_rc != 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
> + errmsg("program \"%s\" failed",
> + copy_state.filename),
> + errdetail_internal("%s", wait_result_to_str(pclose_rc))));
> + }
> + else
> + {
> + if (copy_state.filename != NULL && FreeFile(copy_state.copy_file))
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not close file \"%s\": %m",
> + copy_state.filename)));
> + }

I wonder if these should be an auxiliary function in copy.c to do this.
Surely copy.c itself does pretty much the same thing ...

> +DATA(insert OID = 3353 ( copy_srf PGNSP PGUID 12 1 0 0 0 f f f f f t v u 9 0 2249 "25 16 25 25 25 16 25 25 25" _null_ _null_ _null_ _null_ _null_ copy_srf _null_ _null_ _null_ ));
> +DESCR("set-returning COPY proof of concept");

Why is this marked "proof of concept"? If this is a PoC only, why are
you submitting it as a patch?

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