"Imseih (AWS), Sami" <sims...@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
