Re: Review: Fix snapshot taking inconsistencies

On 2010-10-04 6:19 AM, Steve Singer wrote:
> I also noticed that functions.c is now generating a warning that should be
> easy to clean up.
>
> functions.c: In function 'sql_exec_error_callback':
> functions.c:989: warning: 'es' may be used uninitialized in this function
> functions.c: In function 'fmgr_sql':
> functions.c:712: warning: 'es' is used uninitialized in this function

Those didn't look like actual bugs to me, but fixed in the attached
patch. Thanks.

>> Currently pg_parse_and_rewrite() returns all Query nodes in one huge list.
>> That's not acceptable for this patch since that list is already missing the
>> information we need: when should we take a new snapshot? So the patch breaks
>> the API of pg_parse_and_rewrite() to return a list of lists instead, but I'm
>> not convinced that's a bright idea since third party code might use it, so I
>> suggested adding a new function. Then again, third party code can't use
>> pg_parse_and_rewrite() any way if/when the wCTE patch goes in.
>>
>
> Is there any third party code in particular that your thinking of? I don't
> see anything that says pg_parse_and_rewrite is part of a stable server
> side API (in contrast to SPI or something an third party index access method
> or custom data-type would call).

Nope. I think I grepped contrib/ at some point and none of those were
using pg_parse_and_rewrite() so this is all just speculation. And yes,
it's not really part of any stable API but breaking third party modules
needlessly is not something I want to do. However, in this case there
is no way to avoid breaking them.

My primary concern is that any module using pg_parse_and_rewrite() will
more or less silently break once this patch goes in no matter what we
do. If we leave pg_parse_and_rewrite as is, they will do the wrong
thing (grab a new snapshot for every rewrite product). The problem
might not be noticeable (no reports of EXPLAIN ANALYZE behaving
differently for several years), but once the wCTE patch gets in, it will
be. If we modify pg_parse_and_rewrite like the patch does, modules
start behaving weirdly. So what I'm suggesting is:

- Deprecate pg_parse_and_rewrite(). I have no idea how the project
has done this in the past, but grepping the source code for
"deprecated" suggests that we just remove the function.

- Introduce a new function, specifically designed for SQL functions.
Both callers of pg_parse_and_rewrite (init_sql_fcache and
fmgr_sql_validator) call check_sql_fn_retval after
pg_parse_and_rewrite so I think we could merge those into the new
function.

Does anyone have any concerns about this? Better ideas?

Regards,
Marko Tiikkaja