Обсуждение: Feature: temporary materialized views

Hi!

Sometimes materialized views are used to cache a complex query on
which a client works. But after client disconnects, the materialized
view could be deleted. Regular VIEWs and TABLEs both have support for
temporary versions which get automatically dropped at the end of the
session. It seems it is easy to add the same thing for materialized
views as well. See attached PoC patch.


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 2018-Dec-25, Mitar wrote:

> Sometimes materialized views are used to cache a complex query on
> which a client works. But after client disconnects, the materialized
> view could be deleted. Regular VIEWs and TABLEs both have support for
> temporary versions which get automatically dropped at the end of the
> session. It seems it is easy to add the same thing for materialized
> views as well. See attached PoC patch.

I think MVs that are dropped at session end are a sensible feature.  I
probably wouldn't go as far as allowing ON COMMIT actions, though, so
this much effort is the right amount.

I think if you really want to do this you should just use OptTemp, and
delete OptNoLog.  Of course, you need to add tests and patch the docs.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi!

On Wed, Dec 26, 2018 at 9:00 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> I think MVs that are dropped at session end are a sensible feature.

Thanks.

> I probably wouldn't go as far as allowing ON COMMIT actions, though

I agree. I do not see much usefulness for it. The only use case I can
think of would be to support REFRESH as an ON COMMIT action. That
would be maybe useful in the MV setting. After every transaction in my
session, REFRESH this materialized view.

But personally I do not have an use case for that, so I will leave it
to somebody else. :-)

> I think if you really want to do this you should just use OptTemp, and
> delete OptNoLog.

Sounds good.

OptTemp seems to have a misleading warning in some cases when it is
not used on tables though:

"GLOBAL is deprecated in temporary table creation"

Should we change this language to something else? "GLOBAL is
deprecated in temporary object creation"? Based on grammar it seems to
be used for tables, views, sequences, and soon materialized views.

> Of course, you need to add tests and patch the docs.

Sure.

[1] https://www.postgresql.org/message-id/29165.1545842105%40sss.pgh.pa.us


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 2018-Dec-26, Mitar wrote:

> OptTemp seems to have a misleading warning in some cases when it is
> not used on tables though:
> 
> "GLOBAL is deprecated in temporary table creation"
> 
> Should we change this language to something else? "GLOBAL is
> deprecated in temporary object creation"? Based on grammar it seems to
> be used for tables, views, sequences, and soon materialized views.

I'd just leave those messages alone.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi!

I made a new version of the patch. I added tests and changes to the
docs and made sure various other aspects of this change for as well. I
think this now makes temporary materialized views fully implemented
and that in my view patch is complete. If there is anything else to
add, please let me know, I do not yet have much experience
contributing here. What are next steps? Do I just wait for it to be
included into Commitfest? Do I add it there myself?


Mitar

On Wed, Dec 26, 2018 at 9:00 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2018-Dec-25, Mitar wrote:
>
> > Sometimes materialized views are used to cache a complex query on
> > which a client works. But after client disconnects, the materialized
> > view could be deleted. Regular VIEWs and TABLEs both have support for
> > temporary versions which get automatically dropped at the end of the
> > session. It seems it is easy to add the same thing for materialized
> > views as well. See attached PoC patch.
>
> I think MVs that are dropped at session end are a sensible feature.  I
> probably wouldn't go as far as allowing ON COMMIT actions, though, so
> this much effort is the right amount.
>
> I think if you really want to do this you should just use OptTemp, and
> delete OptNoLog.  Of course, you need to add tests and patch the docs.
>
> --
> Álvaro Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



--
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 2018-Dec-27, Mitar wrote:

> Hi!
> 
> I made a new version of the patch. I added tests and changes to the
> docs and made sure various other aspects of this change for as well. I
> think this now makes temporary materialized views fully implemented
> and that in my view patch is complete. If there is anything else to
> add, please let me know, I do not yet have much experience
> contributing here. What are next steps? Do I just wait for it to be
> included into Commitfest? Do I add it there myself?

Yes, please add it yourself to the commitfest.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi!

Thanks, I did it.

I am attaching a new version of the patch with few more lines added to tests.

I noticed that there is no good summary of the latest patch, so let me
make it here:

So the latest version of the patch adds an option for "temporary"
materialized views. Such materialized views are automatically deleted
at the end of the session. Moreover, it also modifies the materialized
view creation logic so that now if any of the source relations are
temporary, the final materialized view is temporary as well. This now
makes materialized views more aligned with regular views.

Tests test that this really works, that refreshing of such views work,
and that refreshing can also work from a trigger.


Mitar

On Thu, Dec 27, 2018 at 5:15 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2018-Dec-27, Mitar wrote:
>
> > Hi!
> >
> > I made a new version of the patch. I added tests and changes to the
> > docs and made sure various other aspects of this change for as well. I
> > think this now makes temporary materialized views fully implemented
> > and that in my view patch is complete. If there is anything else to
> > add, please let me know, I do not yet have much experience
> > contributing here. What are next steps? Do I just wait for it to be
> > included into Commitfest? Do I add it there myself?
>
> Yes, please add it yourself to the commitfest.
>
> --
> Álvaro Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



--
http://mitar.tnode.com/
https://twitter.com/mitar_m

Hi!

One more version of the patch with more deterministic tests.


Mitar

On Thu, Dec 27, 2018 at 10:35 AM Mitar <mmitar@gmail.com> wrote:
>
> Hi!
>
> Thanks, I did it.
>
> I am attaching a new version of the patch with few more lines added to tests.
>
> I noticed that there is no good summary of the latest patch, so let me
> make it here:
>
> So the latest version of the patch adds an option for "temporary"
> materialized views. Such materialized views are automatically deleted
> at the end of the session. Moreover, it also modifies the materialized
> view creation logic so that now if any of the source relations are
> temporary, the final materialized view is temporary as well. This now
> makes materialized views more aligned with regular views.
>
> Tests test that this really works, that refreshing of such views work,
> and that refreshing can also work from a trigger.
>
>
> Mitar
>
> On Thu, Dec 27, 2018 at 5:15 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >
> > On 2018-Dec-27, Mitar wrote:
> >
> > > Hi!
> > >
> > > I made a new version of the patch. I added tests and changes to the
> > > docs and made sure various other aspects of this change for as well. I
> > > think this now makes temporary materialized views fully implemented
> > > and that in my view patch is complete. If there is anything else to
> > > add, please let me know, I do not yet have much experience
> > > contributing here. What are next steps? Do I just wait for it to be
> > > included into Commitfest? Do I add it there myself?
> >
> > Yes, please add it yourself to the commitfest.
> >
> > --
> > Álvaro Herrera                https://www.2ndQuadrant.com/
> > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>
> --
> http://mitar.tnode.com/
> https://twitter.com/mitar_m



--
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 12/28/18 8:48 AM, Mitar wrote:> One more version of the patch with 
more deterministic tests.

Her is quick initial review. I will do more testing later.

It applies builds and passes the tests.

The feature seems useful and also improves consistency, if we have 
temporary tables and temporary views there should logically also be 
temporary materialized tables.

As for you leaving out ON COMMIT I feel that it is ok since of the 
existing options only really DROP makes any sense (you cannot truncate 
materialized views) and since temporary views do not have any ON COMMIT 
support.

= Comments on the code

The changes to the code are small and look mostly correct.

In create_ctas_internal() why do you copy the relation even when you do 
not modify it?

Is it really ok to just remove SECURITY_RESTRICTED_OPERATION from 
ExecCreateTableAs()? I feel it is there for a good reason and that we 
preferably want to reduce the duration of SECURITY_RESTRICTED_OPERATION 
to only include when we actually execute the query.

Andreas

Hi!

On Fri, Jan 11, 2019 at 8:51 AM Andreas Karlsson <andreas@proxel.se> wrote:
> Her is quick initial review. I will do more testing later.

Thanks for doing the review!

> In create_ctas_internal() why do you copy the relation even when you do
> not modify it?

I was modelling this after code in view.c [1]. I can move copy into the "if".

> Is it really ok to just remove SECURITY_RESTRICTED_OPERATION from
> ExecCreateTableAs()? I feel it is there for a good reason and that we
> preferably want to reduce the duration of SECURITY_RESTRICTED_OPERATION
> to only include when we actually execute the query.

The comment there said that this is not really necessary for security:

"This is not necessary for security, but this keeps the behavior
similar to REFRESH MATERIALIZED VIEW.  Otherwise, one could create a
materialized view not possible to refresh."

Based on my experimentation, this is required to be able to use
temporary materialized views, but it does mean one has to pay
attention from where one can refresh. For example, you cannot refresh
from outside of the current session, because temporary object is not
available there. I have not seen any other example where refresh would
not be possible.

This is why I felt comfortable removing this. Also, no test failed
after removing this.

[1] https://github.com/postgres/postgres/blob/master/src/backend/commands/view.c#L554


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 1/11/19 8:47 PM, Mitar wrote:
>> In create_ctas_internal() why do you copy the relation even when you do
>> not modify it?
> 
> I was modelling this after code in view.c [1]. I can move copy into the "if".

Makes sense.

>> Is it really ok to just remove SECURITY_RESTRICTED_OPERATION from
>> ExecCreateTableAs()? I feel it is there for a good reason and that we
>> preferably want to reduce the duration of SECURITY_RESTRICTED_OPERATION
>> to only include when we actually execute the query.
> 
> The comment there said that this is not really necessary for security:
> 
> "This is not necessary for security, but this keeps the behavior
> similar to REFRESH MATERIALIZED VIEW.  Otherwise, one could create a
> materialized view not possible to refresh."
> 
> Based on my experimentation, this is required to be able to use
> temporary materialized views, but it does mean one has to pay
> attention from where one can refresh. For example, you cannot refresh
> from outside of the current session, because temporary object is not
> available there. I have not seen any other example where refresh would
> not be possible.
> 
> This is why I felt comfortable removing this. Also, no test failed
> after removing this.

Hm, I am still not convinced just removing it is a good idea. Sure, it 
is not a security issue but usability is also important. The question is 
how much this worsens usability and how much extra work it would be to 
keep the restriction.

Btw, if we are going to remove SECURITY_RESTRICTED_OPERATION we should 
remove more code. There is no reason to save and reset the bitmask if we 
do not alter it.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 1/11/19 8:47 PM, Mitar wrote:
>>> Is it really ok to just remove SECURITY_RESTRICTED_OPERATION from
>>> ExecCreateTableAs()?

>> The comment there said that this is not really necessary for security:
>> "This is not necessary for security, but this keeps the behavior
>> similar to REFRESH MATERIALIZED VIEW.  Otherwise, one could create a
>> materialized view not possible to refresh."

> Hm, I am still not convinced just removing it is a good idea. Sure, it 
> is not a security issue but usability is also important.

Indeed.  I don't buy the argument that this should work differently
for temp views.  The fact that they're only accessible in the current
session is no excuse for that: security considerations still matter,
because you can have different privilege contexts within a single
session (consider SECURITY DEFINER functions etc).

What is the stumbling block to just leaving that alone?

            regards, tom lane

On 1/17/19 4:57 PM, Tom Lane wrote:
> Andreas Karlsson <andreas@proxel.se> writes:
>> On 1/11/19 8:47 PM, Mitar wrote:
>>>> Is it really ok to just remove SECURITY_RESTRICTED_OPERATION from
>>>> ExecCreateTableAs()?
> 
>>> The comment there said that this is not really necessary for security:
>>> "This is not necessary for security, but this keeps the behavior
>>> similar to REFRESH MATERIALIZED VIEW.  Otherwise, one could create a
>>> materialized view not possible to refresh."
> 
>> Hm, I am still not convinced just removing it is a good idea. Sure, it
>> is not a security issue but usability is also important.
> 
> Indeed.  I don't buy the argument that this should work differently
> for temp views.  The fact that they're only accessible in the current
> session is no excuse for that: security considerations still matter,
> because you can have different privilege contexts within a single
> session (consider SECURITY DEFINER functions etc).
> 
> What is the stumbling block to just leaving that alone?

I think the issue Mitar ran into is that the temporary materialized view 
is created in the rStartup callback of the receiver which happens after 
SECURITY_RESTRICTED_OPERATION is set in ExecCreateTableAs(), so the 
creation of the view itself is denied.

 From a cursory glance it looks like it would be possible to move the 
setting of SECURITY_RESTRICTED_OPERATION to inside the rStartup 
callabck, other than that the code for resetting the security context 
might get a bit ugly. Do you see any flaws with that solution?

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> On 1/17/19 4:57 PM, Tom Lane wrote:
>> What is the stumbling block to just leaving that alone?

> I think the issue Mitar ran into is that the temporary materialized view 
> is created in the rStartup callback of the receiver which happens after 
> SECURITY_RESTRICTED_OPERATION is set in ExecCreateTableAs(), so the 
> creation of the view itself is denied.

Hm.

>  From a cursory glance it looks like it would be possible to move the 
> setting of SECURITY_RESTRICTED_OPERATION to inside the rStartup 
> callabck, other than that the code for resetting the security context 
> might get a bit ugly. Do you see any flaws with that solution?

Don't think that works: the point here is to restrict what can happen
during planning/execution of the view query, so letting planning and
query startup happen first is no good.

Creating the view object inside the rStartup callback is itself pretty
much of a kluge; you'd expect that to happen earlier.  I think the
reason it was done that way was it was easier to find out the view's
column set there, but I'm sure we can find another way --- doing the
object creation more like a regular view does it is the obvious approach.

            regards, tom lane

On 1/11/19 8:47 PM, Mitar wrote:
> Thanks for doing the review!

I did some functional testing today and everything seems to work as 
expected other than that the tab completion for psql seems to be missing.

Andreas

Hi!

On Thu, Jan 17, 2019 at 9:53 AM Andreas Karlsson <andreas@proxel.se> wrote:
> > What is the stumbling block to just leaving that alone?
>
> I think the issue Mitar ran into is that the temporary materialized view
> is created in the rStartup callback of the receiver which happens after
> SECURITY_RESTRICTED_OPERATION is set in ExecCreateTableAs(), so the
> creation of the view itself is denied.

Yes, the error without that change is:

ERROR:  cannot create temporary table within security-restricted operation


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

Hi!

On Thu, Jan 17, 2019 at 2:40 PM Andreas Karlsson <andreas@proxel.se> wrote:
> I did some functional testing today and everything seems to work as
> expected other than that the tab completion for psql seems to be missing.

Thanks. I can add those as soon as I figure how. :-)

So what are next steps here besides tab autocompletion? It is OK to
remove that security check? If I understand correctly, there are some
general refactoring of code Tom is proposing, but I am not sure if I
am able to do that/understand that.


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 1/18/19 2:53 AM, Mitar wrote:> On Thu, Jan 17, 2019 at 2:40 PM 
Andreas Karlsson <andreas@proxel.se> wrote:
>> I did some functional testing today and everything seems to work as
>> expected other than that the tab completion for psql seems to be missing.
> 
> Thanks. I can add those as soon as I figure how. :-)

These rules are usually pretty easy to add. Just take a look in 
src/bin/psql/tab-complete.c to see how it is usually done.

> So what are next steps here besides tab autocompletion? It is OK to
> remove that security check? If I understand correctly, there are some
> general refactoring of code Tom is proposing, but I am not sure if I
> am able to do that/understand that.

No, I do not think it is ok to remove the check without a compelling 
argument for why the usability we gain from this check is not worth it. 
Additionally I agree with Tom that the way the code is written currently 
is confusing so this refactoring would most likely be a win even without 
your patch.

I might take a stab at refactoring this myself this weekend. Hopefully 
it is not too involved.

Andreas

Hi!

On Fri, Jan 18, 2019 at 7:18 AM Andreas Karlsson <andreas@proxel.se> wrote:
> These rules are usually pretty easy to add. Just take a look in
> src/bin/psql/tab-complete.c to see how it is usually done.

Thanks. I have added the auto-complete and attached a new patch.

> I might take a stab at refactoring this myself this weekend. Hopefully
> it is not too involved.

That would be great! I can afterwards update the patch accordingly.


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 1/17/19 8:31 PM, Tom Lane wrote:
> Creating the view object inside the rStartup callback is itself pretty
> much of a kluge; you'd expect that to happen earlier.  I think the
> reason it was done that way was it was easier to find out the view's
> column set there, but I'm sure we can find another way --- doing the
> object creation more like a regular view does it is the obvious approach.

Here is a a stab at refactoring this so the object creation does not 
happen in a callback. I am not that fond of the new API, but given how 
different all the various callers of CreateIntoRelDestReceiver() are I 
had no better idea.

The idea behind the patch is to always create the empty 
table/materialized view before executing the query and do it in one more 
unified code path, and then later populate it unless NO DATA was 
specified. I feel this makes the code easier to follow.

This patch introduces a small behavioral change, as can be seen from the 
diff in the test suite, where since the object creation is moved earlier 
the CTAS query snapshot will for example see the newly created table. 
The new behavior seems more correct to me, but maybe I am missing some 
unintended consequences.

Andreas

On 1/18/19 8:32 PM, Mitar wrote:
> On Fri, Jan 18, 2019 at 7:18 AM Andreas Karlsson <andreas@proxel.se> wrote:
>> These rules are usually pretty easy to add. Just take a look in
>> src/bin/psql/tab-complete.c to see how it is usually done.
> 
> Thanks. I have added the auto-complete and attached a new patch.

Hm, I do not think we should complete UNLOGGED MATERIALIZED VIEW even 
though it is valid syntax. If you try to create one you will just get an 
error. I am leaning towards removing the existing completion for this, 
because I do not see the point of completing to useless but technically 
valid syntax.

This is the one I think we should probably remove:

      else if (TailMatches("CREATE", "UNLOGGED"))
          COMPLETE_WITH("TABLE", "MATERIALIZED VIEW");

>> I might take a stab at refactoring this myself this weekend. Hopefully
>> it is not too involved.
> 
> That would be great! I can afterwards update the patch accordingly.

I have submitted a first shot at this. Let's see what others think of my 
patch.

Andreas

On 1/21/19 3:31 AM, Andreas Karlsson wrote:
> Here is a a stab at refactoring this so the object creation does not 
> happen in a callback.

Rebased my patch on top of Andres's pluggable storage patches. Plus some 
minor style changes.

Andreas

On Tue, Jan 22, 2019 at 03:10:17AM +0100, Andreas Karlsson wrote:
> On 1/21/19 3:31 AM, Andreas Karlsson wrote:
> > Here is a a stab at refactoring this so the object creation does not
> > happen in a callback.
>
> Rebased my patch on top of Andres's pluggable storage patches. Plus some
> minor style changes.

Taking a note to look at this refactoring bit, which is different from
the temp matview part.  Moved to next CF for now.
--
Michael

On 2/4/19 7:09 AM, Michael Paquier wrote:
> On Tue, Jan 22, 2019 at 03:10:17AM +0100, Andreas Karlsson wrote:
>> On 1/21/19 3:31 AM, Andreas Karlsson wrote:
>>> Here is a a stab at refactoring this so the object creation does not
>>> happen in a callback.
>>
>> Rebased my patch on top of Andres's pluggable storage patches. Plus some
>> minor style changes.
> 
> Taking a note to look at this refactoring bit, which is different from
> the temp matview part.  Moved to next CF for now.

Should I submit it as a separate CF entry or is it easiest if my 
refactoring and Mi Tar's feature are reviewed together?

Andreas

On Mon, Feb 04, 2019 at 04:10:09PM +0100, Andreas Karlsson wrote:
> Should I submit it as a separate CF entry or is it easiest if my refactoring
> and Mi Tar's feature are reviewed together?

The refactoring patch is talking about changing the way objects are
created within a CTAS, which is quite different from what is proposed
on this thread, so in order to attract the correct audience a separate
thread with another CF entry seems more appropriate.

Now...  You have on this thread all the audience which already worked
on 874fe3a.  And I am just looking at this patch, evaluating the
behavior change this is introducing.  Still I would recommend a
separate thread as others may want to comment on that particular
point.
--
Michael

Hi Andreas,

On Tue, Feb 05, 2019 at 12:59:12PM +0900, Michael Paquier wrote:
> Now...  You have on this thread all the audience which already worked
> on 874fe3a.  And I am just looking at this patch, evaluating the
> behavior change this is introducing.  Still I would recommend a
> separate thread as others may want to comment on that particular
> point.

So I have read through your patch, and there are a couple of things
which I think we could simplify more.  Here are my notes:
1) We could remove the into clause from DR_intorel, which is used for
two things:
- Determine the relkind of the relation created.  However the relation
gets created before entering in the executor, and we already know its
OID, so we also know its relkind.
- 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.
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.
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.
--
Michael

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

On 2/5/19 6:56 PM, Andreas Karlsson wrote:
> 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?

In general I do not like how EXPLAIN CREATE TABLE AS uses a separate 
code path than CREATE TABLE AS, because we get weird but mostly harmless 
edge cases like the below and that I do not think that EXPLAIN ANALYZE 
CREATE MATERIALIZED VIEW sets the security context properly.

I am not sure if any of this is worth fixing, but it certainly 
contributed to why I thought that it was hard to design a good API.

postgres=# EXPLAIN ANALYZE CREATE TABLE IF NOT EXISTS bar AS SELECT 1;
                                      QUERY PLAN 

------------------------------------------------------------------------------------
  Result  (cost=0.00..0.01 rows=1 width=4) (actual time=0.001..0.002 
rows=1 loops=1)
  Planning Time: 0.030 ms
  Execution Time: 12.245 ms
(3 rows)

Time: 18.223 ms
postgres=# EXPLAIN ANALYZE CREATE TABLE IF NOT EXISTS bar AS SELECT 1;
ERROR:  relation "bar" already exists
Time: 2.129 ms

Andreas

On Tue, Feb 05, 2019 at 06:56:00PM +0100, Andreas Karlsson wrote:
> 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?

This grammar is documented, so it seems to me that it would be just
annoying for users relying on it to throw an error for a pattern that
simply worked, particularly if a driver layer is using it.

The issue this outlines is that we have a gap in the tests for a
subset of the grammar, which is not a good thing.

If I put Assert(!into->skipData) at the beginning of intorel_startup()
then the main regression test suite is able to pass, both on HEAD and
with your patch.  There is one test for CTAS EXECUTE in prepare.sql,
so let's add a pattern with WITH NO DATA there for the first pattern.
Adding a second test with EXPLAIN SELECT INTO into select_into.sql
also looks like a good thing.

Attached is a patch to do that and close the gap.  With that, we will
be able to check for inconsistencies better when working on the
follow-up patches.  What do you think?
--
Michael

On 2/6/19 10:18 AM, Michael Paquier wrote:
> Attached is a patch to do that and close the gap.  With that, we will
> be able to check for inconsistencies better when working on the
> follow-up patches.  What do you think?

I approve. I was when testing this stuff that I found the IF NOT EXISTS 
issue.

Andreas

On Wed, Feb 06, 2019 at 05:05:56PM +0100, Andreas Karlsson wrote:
> On 2/6/19 10:18 AM, Michael Paquier wrote:
>> Attached is a patch to do that and close the gap.  With that, we will
>> be able to check for inconsistencies better when working on the
>> follow-up patches.  What do you think?
>
> I approve. I was when testing this stuff that I found the IF NOT EXISTS
> issue.

Thanks, I have committed those extra tests to close the gap.
--
Michael

On 2/7/19 2:23 AM, Michael Paquier wrote:
> On Wed, Feb 06, 2019 at 05:05:56PM +0100, Andreas Karlsson wrote:
>> On 2/6/19 10:18 AM, Michael Paquier wrote:
>>> Attached is a patch to do that and close the gap.  With that, we will
>>> be able to check for inconsistencies better when working on the
>>> follow-up patches.  What do you think?
>>
>> I approve. I was when testing this stuff that I found the IF NOT EXISTS
>> issue.
> 
> Thanks, I have committed those extra tests to close the gap.

I think a new patch is required here so I have marked this Waiting on 
Author.  cfbot is certainly not happy and anyone trying to review is 
going to have hard time trying to determine what to review.

Regards,
-- 
-David
david@pgmasters.net

On Thu, Mar 07, 2019 at 10:45:04AM +0200, David Steele wrote:
> I think a new patch is required here so I have marked this Waiting on
> Author.  cfbot is certainly not happy and anyone trying to review is going
> to have hard time trying to determine what to review.

I would recommend to mark this patch as returned with feedback as we
already know that we need to rethink a bit harder the way relations
are created in CTAS, not to mention that the case of EXPLAIN CTAS IF
NOT EXISTS is not correctly handled.  This requires more than three of
work which is what remains until the end of this CF, so v12 is not a
sane target.
--
Michael

On 3/8/19 3:38 AM, Michael Paquier wrote:
> On Thu, Mar 07, 2019 at 10:45:04AM +0200, David Steele wrote:
>> I think a new patch is required here so I have marked this Waiting on
>> Author.  cfbot is certainly not happy and anyone trying to review is going
>> to have hard time trying to determine what to review.
> 
> I would recommend to mark this patch as returned with feedback as we
> already know that we need to rethink a bit harder the way relations
> are created in CTAS, not to mention that the case of EXPLAIN CTAS IF
> NOT EXISTS is not correctly handled.  This requires more than three of
> work which is what remains until the end of this CF, so v12 is not a
> sane target.

OK, I will do that on March 13th if there are no arguments to the contrary.

Regards,
-- 
-David
david@pgmasters.net

On 3/8/19 2:38 AM, Michael Paquier wrote:
> On Thu, Mar 07, 2019 at 10:45:04AM +0200, David Steele wrote:
>> I think a new patch is required here so I have marked this Waiting on
>> Author.  cfbot is certainly not happy and anyone trying to review is going
>> to have hard time trying to determine what to review.
> 
> I would recommend to mark this patch as returned with feedback as we
> already know that we need to rethink a bit harder the way relations
> are created in CTAS, not to mention that the case of EXPLAIN CTAS IF
> NOT EXISTS is not correctly handled.  This requires more than three of
> work which is what remains until the end of this CF, so v12 is not a
> sane target.

Agreed. Even if I could find the time to write a patch for this there is 
no way it would make it into v12.

Andreas

Hi!

I just want to make sure if I understand correctly. But my initial
proposal/patch is currently waiting first for all patches for the
refactoring to happen, which are done by amazing Andreas? This sounds
good to me and I see a lot of progress/work has been done and I am OK
with waiting. Please ping me explicitly if there will be anything I am
expected to do at any point in time.

And just to make sure, these current patches are doing just
refactoring but are not also introducing temporary materialized views
yet? Or is that also done in patches made by Andreas?


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 3/14/19 9:13 AM, Mitar wrote:> I just want to make sure if I 
understand correctly. But my initial
> proposal/patch is currently waiting first for all patches for the
> refactoring to happen, which are done by amazing Andreas? This sounds
> good to me and I see a lot of progress/work has been done and I am OK
> with waiting. Please ping me explicitly if there will be anything I am
> expected to do at any point in time.
> 
> And just to make sure, these current patches are doing just
> refactoring but are not also introducing temporary materialized views
> yet? Or is that also done in patches made by Andreas?

Yeah, your patch is sadly stuck behind the refactoring, and the 
refactoring proved to be harder to do than I initially thought. The 
different code paths for executing CREATE MATERIALIZED VIEW are so 
different that it is hard to find a good common interface.

So there is unfortunately little you can do here other than wait for me 
or someone else to do the refactoring as I cannot see your patch getting 
accepted without keeping the existing restrictions on side effects for 
CREATE MATERIALIZED VIEW.

Andreas

Hi!

On Thu, Mar 14, 2019 at 7:56 AM Andreas Karlsson <andreas@proxel.se> wrote:
> Yeah, your patch is sadly stuck behind the refactoring, and the
> refactoring proved to be harder to do than I initially thought. The
> different code paths for executing CREATE MATERIALIZED VIEW are so
> different that it is hard to find a good common interface.
>
> So there is unfortunately little you can do here other than wait for me
> or someone else to do the refactoring as I cannot see your patch getting
> accepted without keeping the existing restrictions on side effects for
> CREATE MATERIALIZED VIEW.

Sounds good. I will wait.

Thanks.


Mitar

-- 
http://mitar.tnode.com/
https://twitter.com/mitar_m

On 3/15/19 3:19 AM, Mitar wrote:
> 
> On Thu, Mar 14, 2019 at 7:56 AM Andreas Karlsson <andreas@proxel.se> wrote:
>> Yeah, your patch is sadly stuck behind the refactoring, and the
>> refactoring proved to be harder to do than I initially thought. The
>> different code paths for executing CREATE MATERIALIZED VIEW are so
>> different that it is hard to find a good common interface.
>>
>> So there is unfortunately little you can do here other than wait for me
>> or someone else to do the refactoring as I cannot see your patch getting
>> accepted without keeping the existing restrictions on side effects for
>> CREATE MATERIALIZED VIEW.
> 
> Sounds good. I will wait.

This patch has been marked as Returned with Feedback since it is not 
clear when the refactoring it depends on will be done.

You can submit to a future commitfest when you are able to produce a new 
patch.

Regards,
-- 
-David
david@pgmasters.net