Обсуждение: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
Repro:
ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
subrunasowner
---------------
f
(1 row)
It also looks like a change to that field may not cause the
subscription worker to restart. It would be good to add a test for that
case.
Regards,
Jeff Davis
On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> wrote: > > > Repro: > ALTER SUBSCRIPTION s1 SET (run_as_owner = true); > SELECT subrunasowner FROM pg_subscription WHERE subname='s1'; > subrunasowner > --------------- > f > (1 row) > Thanks for reporting. I can also reproduce the issue. I think it's because we didn't reflect the option change on catalog. Here is a small patch 0001 to fix it. > It also looks like a change to that field may not cause the subscription worker to > restart. It would be good to add a test for that case. Currently, the changes on run_as_owner won't cause the worker to restart because we don't need to rebuild the connection in this case. The option change will be caught by apply worker in next loop and the later changes will be applied using the new option. the 0002 patch adds a test to verfiy it, just to show how it behaves. Best Regards, Hou zj
Вложения
On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> wrote: > > > > > > Repro: > > ALTER SUBSCRIPTION s1 SET (run_as_owner = true); > > SELECT subrunasowner FROM pg_subscription WHERE subname='s1'; > > subrunasowner > > --------------- > > f > > (1 row) > > > > Thanks for reporting. I can also reproduce the issue. I think it's because we > didn't reflect the option change on catalog. Here is a small patch 0001 to fix it. > Your fix looks good to me. > > It also looks like a change to that field may not cause the subscription worker to > > restart. It would be good to add a test for that case. > > Currently, the changes on run_as_owner won't cause the worker to restart > because we don't need to rebuild the connection in this case. The option change > will be caught by apply worker in next loop and the later changes will be > applied using the new option. the 0002 patch adds a test to verfiy it, just to > show how it behaves. > Is there a reason to not include 0002 in the commit? Shall we push this before 16 or wait for it? I don't if we have enough time or if it is worth pushing at the last minute. I can take care of pushing this tomorrow morning if we decide that it is okay to go ahead with this unless Jeff or Robert pushes it. -- With Regards, Amit Kapila.
On 2023-Sep-11, Amit Kapila wrote: > Shall we push this before 16 or wait for it? I don't if we have enough > time or if it is worth pushing at the last minute. I can take care of > pushing this tomorrow morning if we decide that it is okay to go ahead > with this unless Jeff or Robert pushes it. You should not push anything to the 16 branch until you see the tag come through tomorrow. There is not enough time for a buildfarm round. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Al principio era UNIX, y UNIX habló y dijo: "Hello world\n". No dijo "Hello New Jersey\n", ni "Hello USA\n".
On Monday, September 11, 2023 7:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> > wrote: > > > > On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> > wrote: > > > > > > > > > Repro: > > > ALTER SUBSCRIPTION s1 SET (run_as_owner = true); > > > SELECT subrunasowner FROM pg_subscription WHERE subname='s1'; > > > subrunasowner > > > --------------- > > > f > > > (1 row) > > > > > > > Thanks for reporting. I can also reproduce the issue. I think it's > > because we didn't reflect the option change on catalog. Here is a small patch > 0001 to fix it. > > > > Your fix looks good to me. Thanks for reviewing. > > > > It also looks like a change to that field may not cause the > > > subscription worker to restart. It would be good to add a test for that case. > > > > Currently, the changes on run_as_owner won't cause the worker to > > restart because we don't need to rebuild the connection in this case. > > The option change will be caught by apply worker in next loop and the > > later changes will be applied using the new option. the 0002 patch > > adds a test to verfiy it, just to show how it behaves. > > > > Is there a reason to not include 0002 in the commit? No, I think it's fine to include 0002. > > Shall we push this before 16 or wait for it? I don't if we have enough time or if it > is worth pushing at the last minute. I can take care of pushing this tomorrow > morning if we decide that it is okay to go ahead with this unless Jeff or Robert > pushes it. I saw the "Stamp 16.0" has came in, so it seems we can only include this fix in 16.1. Best Regards, Hou zj
On Tue, Sep 12, 2023 at 7:10 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > On Monday, September 11, 2023 7:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> > > wrote: > > > > > > On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> > > wrote: > > > > > > > > > > > > Repro: > > > > ALTER SUBSCRIPTION s1 SET (run_as_owner = true); > > > > SELECT subrunasowner FROM pg_subscription WHERE subname='s1'; > > > > subrunasowner > > > > --------------- > > > > f > > > > (1 row) > > > > > > > > > > Thanks for reporting. I can also reproduce the issue. I think it's > > > because we didn't reflect the option change on catalog. Here is a small patch > > 0001 to fix it. > > > > > > > Your fix looks good to me. > > Thanks for reviewing. > > > > > > > It also looks like a change to that field may not cause the > > > > subscription worker to restart. It would be good to add a test for that case. > > > > > > Currently, the changes on run_as_owner won't cause the worker to > > > restart because we don't need to rebuild the connection in this case. > > > The option change will be caught by apply worker in next loop and the > > > later changes will be applied using the new option. the 0002 patch > > > adds a test to verfiy it, just to show how it behaves. > > > > > > > Is there a reason to not include 0002 in the commit? > > No, I think it's fine to include 0002. > Pushed by slightly adjusting the comments in the test case. -- With Regards, Amit Kapila.