From: Tom Lane <tgl@sss.pgh.pa.us>
To: "Imseih (AWS), Sami" <simseih@amazon.com>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
In-reply-to: <0AA8F2B1-7855-4F25-BD6C-83CB85722120@amazon.com>
References: <2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com>
 <49272.1685708176@sss.pgh.pa.us>
 <0AA8F2B1-7855-4F25-BD6C-83CB85722120@amazon.com>
Comments: In-reply-to "Imseih (AWS), Sami" <simseih@amazon.com>
	message dated "Fri, 02 Jun 2023 21:26:50 -0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3977000.1685900168.1@sss.pgh.pa.us>
Date: Sun, 04 Jun 2023 13:36:08 -0400
Message-ID: <3977001.1685900168@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3977001.1685900168%40sss.pgh.pa.us>
Precedence: bulk

"Imseih (AWS), Sami" <simseih@amazon.com> writes:
> Some minor comments I have:

> 1/

> + agginfo[i].aggfn.postponed_def = false;	/* might get set during sort */

> This is probably not needed as it seems that we can only
> get into this situation when function dependencies are tracked.

That's just to keep from leaving an undefined field in the
DumpableObject struct.  You are very likely right that nothing
would examine that field today, but that seems like a poor
excuse for not initializing it.

> 2/

> The following description makes more sense. 

> + * section.  This is sufficient to handle cases where a function depends on
> + * some constraints, as can happen if a BEGIN ATOMIC function 
> + * references a constraint directly.

I did not think this was an improvement.  For one thing, I doubt
that BEGIN ATOMIC is essential to cause the problem.  I didn't
prove the point by making a test case, but a "RETURN expression"
function could probably trip over it too by putting a sub-SELECT
with GROUP BY into the expression.  Also, your wording promises
more about what cases we can handle than I think is justified.

> 3/

> The docs in https://www.postgresql.org/docs/current/ddl-depend.html
> should be updated.

Right.  In general, I thought that the new-style-SQL-functions patch
was very slipshod about updating the docs, and omitted touching
many places where it would be appropriate to mention the new style,
or even flat-out replace examples with new syntax.  I did something
about this particular point, but perhaps someone would like to look
around and work on that topic?

			regards, tom lane