Re: [HACKERS] COPY as a set returning function - Mailing list pgsql-hackers
| From | Corey Huinker |
|---|---|
| Subject | Re: [HACKERS] COPY as a set returning function |
| Date | |
| Msg-id | CADkLM=fPkVsC9kb1rsx+wez=K2reopJDgXKCWQhjw2wOsBd95w@mail.gmail.com Whole thread Raw |
| In response to | Re: [HACKERS] COPY as a set returning function (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Responses |
Re: [HACKERS] COPY as a set returning function
(Corey Huinker <corey.huinker@gmail.com>)
|
| List | pgsql-hackers |
> + /* 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.
If I understand you correctly, COPY (via BeginCopyFrom) itself relies on having a relation in pg_class to reference for attributes.
In this case, there is no such relation. So I'd have to fake a relcache entry, or refactor BeginCopyFrom() to extract a ReturnSetInfo from the Relation and pass that along to a new function BeginCopyFromReturnSet. I'm happy to go that route if you think it's a good idea.
> + 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.
Good to know!
> + }
> +
> + /* 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 ...
Yes. This got started as a patch to core because not all of the parts of COPY are externally callable, and aren't broken down in a way that allowed for use in a SRF.
I'll get to work on these suggestions.
pgsql-hackers by date: