On 2/5/19 12:36 PM, Michael Paquier wrote:> - skipData is visibly always
false.
> We may want to keep skipData to have an assertion at the beginning of
> inforel_startup for sanity purposes though.
This is not true in this version of the patch. The following two cases
would crash if we add such an assertion:
EXPLAIN ANALYZE CREATE TABLE foo AS SELECT 1 WITH NO DATA;
and
PREPARE s AS SELECT 1;
CREATE TABLE bar AS EXECUTE s WITH NO DATA;
since they both still run the setup and tear down steps of the executor.
I guess that I could fix that for the second case as soon as I
understand how much of the portal stuff can be skipped in
ExecuteQuery(). But I am not sure what we should do with EXPLAIN ANALYZE
... NO DATA. It feels like a contraction to me. Should we just raise an
error? Or should we try to preserve the current behavior where you see
something like the below?
QUERY PLAN
-----------------------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4) (never executed)
Planning Time: 0.040 ms
Execution Time: 0.002 ms
(3 rows)
> 2) DefineIntoRelForDestReceiver is just a wrapper for
> create_ctas_nodata, so we had better just merge both of them and
> expose directly the routine creating the relation definition, so the
> new interface is a bit awkward.
Agreed, the API is awakward as it is now but it was the least awkward
one I managed to design. But I think if we fix the issue above then it
might be possible to create a less awkward API.
> 3) The part about the regression diff is well... Expected... We may
> want a comment about that. We could consider as well adding a
> regression test inspired from REINDEX SCHEMA to show that the CTAS is
> created before the data is actually filled in.
Yeah, that sounds like a good idea.
Andreas