Re: Review: Fix snapshot taking inconsistencies

On 2010-10-03 5:08 AM +0300, Steve Singer wrote:
> The patch applies against master (a13f12b3a18da0a61571cb134fdecea03a10d6f)
>
>
> However initdb fails with:
>
> FATAL:  return type mismatch in function declared to return record
> DETAIL:  Function's final statement must be SELECT or INSERT/UPDATE/DELETE
> RETURNING.
> CONTEXT:  SQL function "ts_debug"
>
> If I run initdb without the patch applied then apply the patch I can test
> the patch.  However the regression tests won't run with the patch applied
> because they call initdb.

Hmm..  I can't reproduce this.  What platform are you on?

> The patch does not include any regression tests. I don't think we have other
> tests that (can?) test concurrency patterns like this.

Right, we can't, at least not yet.

> Are there any dangers:  Per Marko's comments on the most recent patch:
> "This patch still silently breaks  pg_parse_and_rewrite()..." this still
> seems unresolved.  Marko proposed replacing this with something new for SQL
> functions.  Unfortunately I don't see this as having been followed up on.
>
> I also don't have enough understanding of the code to see exactly how/why it
> was broken or what would be involved in fixing it.

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.


Regards,
Marko Tiikkaja