Re: Fail Fast In CTAS/CMV If Relation Already Exists To Avoid Unnecessary Rewrite, Planning Costs

Thanks for taking a look at this.

On Fri, Dec 11, 2020 at 6:33 AM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
>
> > Currently, for CTAS or CREATE MATERIALIZED VIEW(CMV) without if-not-exists
> > clause, the existence of the relation gets checked during the execution
> > of the select part and an error is thrown there.
> > All the unnecessary rewrite and planning for the select part would have
> > happened just to fail later. However, if if-not-exists clause is present,
> > then a notice is issued and returned immediately without any further rewrite
> > or planning for . This seems somewhat inconsistent to me.
> >
> > I propose to check the relation existence early in ExecCreateTableAs() as
> > well as in ExplainOneUtility() and throw an error in case it exists already
> > to avoid unnecessary rewrite, planning and execution of the select part.
> >
> > Attaching a patch. Note that I have not added any test cases as the existing
> > test cases in create_table.sql and matview.sql would cover the code.
> >
> > Thoughts?
>
> Personally, I think it make sense, as other CMD(such as create extension/index ...) throw that error
> before any further operation too.
>
> I am just a little worried about the behavior change of [explain CTAS].
> May be someone will complain the change from normal explaininfo to error output.

I think we are clear with the patch for plain i.e. non-EXPLAIN and
EXPLAIN ANALYZE CTAS/CMV cases.

The behaviour for EXPLAIN is as follows:

1)EXPLAIN without ANALYZE, without patch: select part is planned(note
that the relations in the select part are checked for their existence
while planning, fails any of them don't exist) , relation(CTAS/CMV
being created) existence is not checked as we will not create the
relation and execute the plan.

2)EXPLAIN with ANALYZE, without patch: select part is planned, as we
execute the plan, relation(CTAS/CMV) existence is checked during the
execution and fails there if it exists.

3) EXPLAIN without ANALYZE, with patch: relation(CTAS/CMV) existence
is checked before the planning and fails if it exists, without going
further to the planning for select part.

4)EXPLAIN with ANALYZE, with patch: relation(CTAS/CMV) existence is
checked before the rewrite, planning and fails if it exists, without
going further.

IMO, let's not change the 1) behaviour to 3) with the patch. If
agreed, I can do the following way in ExplainOneUtility and will add a
comment on why we are doing this.

if (es->analyze)
(void) CheckRelExistenceInCTAS(ctas, true);

Thoughts?

> And I took a look into the patch.
>
> + StringInfoData emsg;
> +
> + initStringInfo(&emsg);
> +
> + if (level == NOTICE)
> + appendStringInfo(&emsg,
>
> Using variable emsg and level seems a little complicated to me.
> How about just:
>
> if (!is_explain && ctas->if_not_exists)
> ereport(NOTICE,xxx
> else
> ereport(ERROR,xxx

I will modify it in the next version of the patch which I plan to send
once agreed on the above point.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com