public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Sutou Kouhei <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: Fri, 9 Feb 2024 11:27:05 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi,
On 2024-02-09 13:21:34 +0900, Michael Paquier wrote:
> +static void
> +CopyFromTextInFunc(CopyFromState cstate, Oid atttypid,
> + FmgrInfo *finfo, Oid *typioparam)
> +{
> + Oid func_oid;
> +
> + getTypeInputInfo(atttypid, &func_oid, typioparam);
> + fmgr_info(func_oid, finfo);
> +}
FWIW, we should really change the copy code to initialize FunctionCallInfoData
instead of re-initializing that on every call, realy makes a difference
performance wise.
> +/*
> + * CopyFromTextStart
> + *
> + * Start of COPY FROM for text/CSV format.
> + */
> +static void
> +CopyFromTextStart(CopyFromState cstate, TupleDesc tupDesc)
> +{
> + AttrNumber attr_count;
> +
> + /*
> + * If encoding conversion is needed, we need another buffer to hold the
> + * converted input data. Otherwise, we can just point input_buf to the
> + * same buffer as raw_buf.
> + */
> + if (cstate->need_transcoding)
> + {
> + cstate->input_buf = (char *) palloc(INPUT_BUF_SIZE + 1);
> + cstate->input_buf_index = cstate->input_buf_len = 0;
> + }
> + else
> + cstate->input_buf = cstate->raw_buf;
> + cstate->input_reached_eof = false;
> +
> + initStringInfo(&cstate->line_buf);
Seems kinda odd that we have a supposedly extensible API that then stores all
this stuff in the non-extensible CopyFromState.
> + /* create workspace for CopyReadAttributes results */
> + attr_count = list_length(cstate->attnumlist);
> + cstate->max_fields = attr_count;
Why is this here? This seems like generic code, not text format specific.
> + cstate->raw_fields = (char **) palloc(attr_count * sizeof(char *));
> + /* Set read attribute callback */
> + if (cstate->opts.csv_mode)
> + cstate->copy_read_attributes = CopyReadAttributesCSV;
> + else
> + cstate->copy_read_attributes = CopyReadAttributesText;
> +}
Isn't this precisely repeating the mistake of 2889fd23be56?
And, why is this done here? Shouldn't this decision have been made prior to
even calling CopyFromTextStart()?
> +/*
> + * CopyFromTextOneRow
> + *
> + * Copy one row to a set of `values` and `nulls` for the text and CSV
> + * formats.
> + */
I'm very doubtful it's a good idea to combine text and CSV here. They have
basically no shared parsing code, so what's the point in sending them through
one input routine?
> +bool
> +CopyFromTextOneRow(CopyFromState cstate,
> + ExprContext *econtext,
> + Datum *values,
> + bool *nulls)
> +{
> + TupleDesc tupDesc;
> + AttrNumber attr_count;
> + FmgrInfo *in_functions = cstate->in_functions;
> + Oid *typioparams = cstate->typioparams;
> + ExprState **defexprs = cstate->defexprs;
> + char **field_strings;
> + ListCell *cur;
> + int fldct;
> + int fieldno;
> + char *string;
> +
> + tupDesc = RelationGetDescr(cstate->rel);
> + attr_count = list_length(cstate->attnumlist);
> +
> + /* read raw fields in the next line */
> + if (!NextCopyFromRawFields(cstate, &field_strings, &fldct))
> + return false;
> +
> + /* check for overflowing fields */
> + if (attr_count > 0 && fldct > attr_count)
> + ereport(ERROR,
> + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> + errmsg("extra data after last expected column")));
It bothers me that we look to be ending up with different error handling
across the various output formats, particularly if they're ending up in
extensions. That'll make it harder to evolve this code in the future.
> + fieldno = 0;
> +
> + /* Loop to read the user attributes on the line. */
> + foreach(cur, cstate->attnumlist)
> + {
> + int attnum = lfirst_int(cur);
> + int m = attnum - 1;
> + Form_pg_attribute att = TupleDescAttr(tupDesc, m);
> +
> + if (fieldno >= fldct)
> + ereport(ERROR,
> + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> + errmsg("missing data for column \"%s\"",
> + NameStr(att->attname))));
> + string = field_strings[fieldno++];
> +
> + if (cstate->convert_select_flags &&
> + !cstate->convert_select_flags[m])
> + {
> + /* ignore input field, leaving column as NULL */
> + continue;
> + }
> +
> + cstate->cur_attname = NameStr(att->attname);
> + cstate->cur_attval = string;
> +
> + if (cstate->opts.csv_mode)
> + {
More unfortunate intermingling of multiple formats in a single routine.
> +
> + if (cstate->defaults[m])
> + {
> + /*
> + * The caller must supply econtext and have switched into the
> + * per-tuple memory context in it.
> + */
> + Assert(econtext != NULL);
> + Assert(CurrentMemoryContext == econtext->ecxt_per_tuple_memory);
> +
> + values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
> + }
I don't think it's good that we end up with this code in different copy
implementations.
Greetings,
Andres Freund
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox