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

On Fri, Dec 18, 2020 at 8:15 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> On Fri, Dec 18, 2020 at 7:18 AM Michael Paquier <michael@paquier.xyz> wrote:
> > On Thu, Dec 17, 2020 at 03:06:59PM +0530, Bharath Rupireddy wrote:
> > > The behavior of the ctas/cmv, in case the relation already exists is as
> > > shown in [1]. The things that have been changed with the patch are: 1) In
> > > any case we do not rewrite or plan the select part if the relation already
> > > exists 2) For explain ctas/cmv (without analyze), now the relation
> > > existence is checked early and the error is thrown as highlighted in [1].
> > >
> > > With patch, there is no behavioral change(from that of master branch) in
> > > explain analyze ctas/cmv with if-not-exists i.e. error is thrown not the
> > > notice.
> > >
> > > Thoughts?
> >
> > HEAD is already a mixed bad of behaviors, and the set of results you
> > are presenting here is giving a similar impression.  It brings in some
> > sanity by just ignoring the effects of the IF NOT EXISTS clause all
> > the time still that's not consistent with the queries not using
> > EXPLAIN.
>
> I tried to make it consistent by issuing NOTICE (not an error) even
> for EXPLAIN/EXPLAIN ANALYZE IF NOT EXISTS case. If issue notice and
> exit from the ExplainOneUtility, we could output an empty plan to the
> user because, by now ExplainResultDesc would have been called at the
> start of the explain via PortalStart(). I didn't find a clean way of
> coding if we are not okay to show notice and empty plan to the user.
>
> Any suggestions on achieving above?
>
> > Hmm.  Looking for similar behaviors, I can see one case in
> > select_into.sql where we just never execute the plan when using WITH
> > NO DATA but still show the plan, meaning that the query gets planned
> > but it just gets marked as "(never executed)" if attempting to use
> > ANALYZE.
>
> Yes, with no data we would see "(never executed)" for explain analyze
> if the relation does not already exist. If the relation does exist,
> then the error/notice.
>
> >There may be use cases for that as the user directly asked directly for an EXPLAIN.
>
> IMHO, in any case checking for the existence of the relations
> specified in a query is must before we output something to the user.
> For instance, the query "explain select * from non_existent_tbl;"
> where non_existent_tbl doesn't exist, throws an error. Similarly,
> "explain create table already_existing_tbl as select * from
> another_tbl;" where the table ctas/select into trying to create
> already exists, should also throw error. But that's not happening
> currently on master. Which seems to be a problem to me. So, with the
> patch proposed here, we error out in this case.
>
> If the user really wants to see the explain plan, then he/she should
> use the correct query.
>
> > Note: the patch needs tests for all the patterns you would like to
> > stress.  This way it is easier to follow the patterns that are
> > changing with your patch and compare them with the HEAD behavior (like
> > looking at the diffs with the tests of the patch, but without the
> > diffs in src/backend/).
>
> Sure, I will add test cases and post v3 patch.

Attaching v3 patch that also contains test cases. Please review it further.

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