Feature: temporary materialized views

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]: /messages/by-id/29165.1545842105@sss.pgh.pa.us

Mitar

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

st 26. 12. 2018 v 18:20 odesílatel Mitar <mmitar@gmail.com> napsal:

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.

This message is wrong - probably better "GLOBAL temporary tables are not
supported"

Regards

Pavel

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]https://github.com/postgres/postgres/blob/master/src/backend/commands/view.c#L554. 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