On Wed, 26 Nov 2025 at 20:29, Bernice Southey <[email protected]> wrote: > > Bernice Southey <[email protected]> wrote: > > I went through the history and it seemed to me the repeat rewrite was > > accidental because of the two ways this method can recurse. > I mean the repeat rewrite of the cteList was accidental, not the > repeat rewrite of views. I couldn't think why a view would mean a > cteList needed re-rewriting, but I know very little about view rules.
I agree that the repeat rewrites look accidental. However, I've been thinking about this some more, and I realised that the patch as written isn't correct. I think it is true that RewriteQuery() doesn't need to rewrite individual CTEs multiple times. However, that doesn't mean that it doesn't need to process the cteList at all below the top-level. The problem is that rule actions may add additional CTEs to the list, which do need to be processed when recursing. Here's a simple, dumb example: --- drop table if exists t1, t1a, t1b, t2, t2a; create table t1 (a int); create table t1a (a int); create table t1b (a int); create rule t1r as on insert to t1 do instead with x as (insert into t1a values (1) returning *) insert into t1b select a from x returning *; create table t2 (a int); create table t2a (a int); create rule t2r as on insert to t2 do instead with xx as (insert into t1 values (1) returning *) insert into t2a select a from xx returning *; insert into t2 values (1); --- Both rules should be triggered, resulting in 1 row in each of t1a, t1b, and t2a, which is what happens in HEAD, but with the patch, it fails to fire rule t1r when it recurses, so you get a row in t1 instead. This could probably be fixed up fairly easily, by tracking how many of the CTEs in the list have been processed, and only processing the new ones, when recursing. On the other hand, perhaps making rewriteTargetListIU() idempotent would be a more robust solution. I've not looked in detail at what would be needed for that though. Regards, Dean
