Disclaimer: I had never seen this patch before. I did not participate in this feature design. I did not discuss this review with the author or anybody in 2ndQuadrant. I do not have any particular affective bonds with its author. I did not receive payment nor goods in exchange for this review. I encourage others to review this patch, and all other patches in the current commitfest and all future commitfests.
Now that the TupleTableSlot work has landed, the API of ExecComputeStoredGenerated is clearly inadequate. This should be adjusted to work with the slot only, and not generate a heap tuple at all -- if the calling code needs the heap tuple, have that code generate that from the slot. (Example problem: ExecConstraints runs using the slot, not the heap tuple.) The pg_dump tests verify a virtual generated column, but not a virtual stored column. It'd be good to have one for the latter. The tables in new test "generated" appear not to be dropped, which is good to test pg_upgrade as well as pg_dump; I'd add a comment that this is on purpose, lest someone else add DROP lines there later. I think some tests for logical replication would be good as well. It's unclear why you made generated columns on partitions unsupported. I'd fix the limitation if possible, but if not, at least document it. (I particularly notice that partition key is marked as unsupported in the regression test. Consider partitioning on a SERIAL column, this is clearly something that users will expect to work.) About your catalog representation question: On 2018-Oct-30, Peter Eisentraut wrote: > 1. (current implementation) New column attgenerated contains 's' for > STORED, 'v' for VIRTUAL, '\0' for nothing. atthasdef means "there is > something in pg_attrdef for this column". So a generated column would > have atthasdef = true, and attgenerated = s/v. A traditional default > would have atthasdef = true and attgenerated = '\0'. The advantage is > that this is easiest to implement and the internal representation is the > most useful and straightforward. The disadvantage is that old client > code that wants to detect whether a column has a default would need to > be changed (otherwise it would interpret a generated column as having a > default value instead). I think this is a reasonable implementation. Client code that is confused by this will obviously have to be updated, but if it isn't, the bug is not serious. I would clearly not go to the extreme trouble that #2 poses just to avoid this problem. That said, I think your "radical alternative" #3 is better. Maybe we want to avoid multiple compatibility breaks, so we'd go with 3 for pg12 instead of doing 1 now and changing it again later. Like Robert, I would use something other than '\0' anyhow. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
