reshke commented on PR #725: URL: https://github.com/apache/cloudberry/pull/725#issuecomment-2490646084
Hi! some initial review: 1) Patch miss tab-completion for DYNAMIC feature. 2) When trying to modify dynamic table ``` reshke=# insert into td values(1, 1); ERROR: cannot change materialized view "td" ``` Maybe some better error message here? we can check for relisdynamic and provide "cannot directly change dynamic table " with some errdetail, like "DETAIL: DYNAMIC TABLE data is automatically populated from its source query." 3) Im not sure if this a problem, but example: ``` reshke=# alter materialized view td rename TO tdd; ALTER MATERIALIZED VIEW ``` td here is dynamic table. This all comes from fact that dynamic table relkind is 'm' (mat. view). I'm not entirely sure if this design is good. Although the implementation is undoubtedly simpler in this manner, it is at least perplexing that changing a dynamic table requires M.V. SQL syntax. Another case here: ``` reshke=# drop dynamic table tdd; DROP DYNAMIC TABLE reshke=# drop materialized view tdd ; DROP MATERIALIZED VIEW ``` Both succeeds. Maybe we should create this relation as `CREATE DYNAMIC MATERIALIZED VIEW ` not TABLE? This should fix a lot of SQL ambiguity problems. In any case, we should add this ALTER pattern to regression tests and have document them in some form. I will take another look later. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
