Re: Alias collision in `refresh materialized view concurrently`

On Tue, Jun 1, 2021 at 5:24 PM Bernd Helmle <mailings@oopsware.de> wrote:
>
> Am Dienstag, dem 01.06.2021 um 13:13 +0530 schrieb Bharath Rupireddy:
> > I used MyProcPid which seems more random than MyBackendId (which is
> > just a number like 1,2,3...). Even with this, someone could argue
> > that
> > they can look at the backend PID, use it in the materialized view
> > names just to trick the server. I'm not sure if anyone would want to
> > do this.
> >
>
> A generated query likely uses just an incremented value derived from
> somewhere and in my opinion 1,2,3 makes it more likely that you get a
> chance for collisions if you managed to get the same alias prefix
> somehow. So +1 with the MyProcPid...

Thanks.

> > I used the existing function make_temptable_name_n to prepare the
> > alias names. The advantage of this is that the code looks cleaner,
> > but
> > it leaks memory, 1KB string for each call of the function. This is
> > also true with the existing usage of the function. Now, we will have
> > 5
> > make_temptable_name_n function calls leaking 5KB memory. And we also
> > have quote_qualified_identifier leaking memory, 2 function calls,
> > 2KB.
> > So, in total, these two functions will leak 7KB of memory (with the
> > patch).
> >
> > Shall I pfree the memory for all the strings returned by the
> > functions
> > make_temptable_name_n and quote_qualified_identifier? The problem is
> > that pfree isn't cheaper.
> > Or shall we leave it as is so that the memory will be freed up by the
> > context?
> >
>
> afaics the memory context is deleted after execution immediately, so
> i'd assume it's okay.

Yes, the refresh operation happens in the "PortalContext", which gets
destroyed at the end of the query in PortalDrop.

PSA v3 patch. I added a commit message and made some cosmetic adjustments.

With Regards,
Bharath Rupireddy.