Thread: [PATCH] pgarchives: Bugfix: missing ids in pglister_sync
groupid and listid are not explicitly set in pglister_sync's queries, which causes the script to fail as these fields are NOT NULL and postgresql default configuration does not auto-fill id fields. This patch sets them explicitly by reusing the ones obtained from pglister. -- Célestin Matte
Attachment
On Fri, Jan 28, 2022 at 6:52 PM Célestin Matte <celestin.matte@cmatte.me> wrote: > > groupid and listid are not explicitly set in pglister_sync's queries, which > causes the script to fail as these fields are NOT NULL and postgresql default > configuration does not auto-fill id fields. > > This patch sets them explicitly by reusing the ones obtained from pglister. These fields are supposed to be serial fields, not pure integers, I think. This feels like some very old legacy that hasn't been properly fixed. I think the attached patch is more correct (I've backpatched it into the 0001 migration because making it a separate migration makes it somewhat destructive -- it's easier to just change it manually on existing installs if you have one) -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Attachment
> I think the attached patch is more correct (I've backpatched it into > the 0001 migration because making it a separate migration makes it > somewhat destructive -- it's easier to just change it manually on > existing installs if you have one) Thanks! Can it be pushed into the git repository, or does it need testing or something? -- Célestin Matte
>> I think the attached patch is more correct (I've backpatched it into >> the 0001 migration because making it a separate migration makes it >> somewhat destructive -- it's easier to just change it manually on >> existing installs if you have one) > > Thanks! Can it be pushed into the git repository, or does it need testing or something? > I understand that changing id fields to AutoFields can be destructive for existing install, but on the other hand, the currentstate of the repository is problematic as it causes pglister_sync to fail by default. Another solution could be to add sequences directly in postgres using RunSQL() in a migration to turn these fields into defacto serial fields. This is a bit hacky, but non-destructive. What do you think? -- Célestin Matte
On Thu, Feb 3, 2022 at 2:52 PM Célestin Matte <celestin.matte@cmatte.me> wrote: > > >> I think the attached patch is more correct (I've backpatched it into > >> the 0001 migration because making it a separate migration makes it > >> somewhat destructive -- it's easier to just change it manually on > >> existing installs if you have one) > > > > Thanks! Can it be pushed into the git repository, or does it need testing or something? I was mainly waiting for comments on if this patch works for you, or if others have any other ideas around it. Have you been able to test it on your end? > I understand that changing id fields to AutoFields can be destructive for existing install, but on the other hand, thecurrent state of the repository is problematic as it causes pglister_sync to fail by default. > Another solution could be to add sequences directly in postgres using RunSQL() in a migration to turn these fields intode facto serial fields. This is a bit hacky, but non-destructive. What do you think? I'll have no problem forcing it through on the production instances we have, I can just do that sync step manually. From the side of us (postgresql.org infrastructure) we don't need it to be an automated roll forward, we can just backpatch it into the 0001 migration and be done with it. That'll make it correct for all new installs, and we'll have to manually fix the existing ones. Would that method work for you as well? -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
> I was mainly waiting for comments on if this patch works for you, or > if others have any other ideas around it. > > Have you been able to test it on your end? Just tested, it works fine > I'll have no problem forcing it through on the production instances we > have, I can just do that sync step manually. From the side of us > (postgresql.org infrastructure) we don't need it to be an automated > roll forward, we can just backpatch it into the 0001 migration and be > done with it. That'll make it correct for all new installs, and we'll > have to manually fix the existing ones. > > Would that method work for you as well? > That works for me! -- Célestin Matte
Hello, This patch was never merge into master, the issue still exists. I think it's been forgotten. Can it be merged now? On 30/01/2022 13:32, Magnus Hagander wrote: > On Fri, Jan 28, 2022 at 6:52 PM Célestin Matte <celestin.matte@cmatte.me> wrote: >> >> groupid and listid are not explicitly set in pglister_sync's queries, which >> causes the script to fail as these fields are NOT NULL and postgresql default >> configuration does not auto-fill id fields. >> >> This patch sets them explicitly by reusing the ones obtained from pglister. > > These fields are supposed to be serial fields, not pure integers, I > think. This feels like some very old legacy that hasn't been properly > fixed. > > I think the attached patch is more correct (I've backpatched it into > the 0001 migration because making it a separate migration makes it > somewhat destructive -- it's easier to just change it manually on > existing installs if you have one) > -- Célestin Matte