On Wed, Jan 25, 2017 at 11:57 AM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
> 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
>
Feel free to mark it returned as feedback. The concept didn't generate as
much enthusiasm as I had hoped, so I think the right thing to do now is
scale it back to a patch that makes CopyFromRawFields() externally visible
so that extensions can use them.
