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