Ubuntu
postgresql-common package

pg_conftool mistakes regular comments for commented config options

Bug #2007794 reported by wycitox incox
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
postgresql-common (Ubuntu)
Fix Released
Medium
Athos Ribeiro
Bionic
Won't Fix
Low
Athos Ribeiro
Focal
Won't Fix
Low
Athos Ribeiro
Jammy
Fix Committed
Low
Athos Ribeiro
Kinetic
Fix Committed
Low
Athos Ribeiro
Lunar
Fix Released
Medium
Athos Ribeiro

Bug Description

[ Impact ]

pg_conftool mistakes regular comments starting with a configuration option for actual commented configuration options.

While the issue does not impact any software features or its usability, it may lead to the deletion of useful comments or documentation in configuration files.

[ Test Plan ]

The proposed change includes new tests to cover the most basic cases the regular expression was failing to deal with.

On top of that, we can verify the fix by

0) Installing postgresql-$PG_VERSION

1) appending

# test_config this is just a comment

to the /etc/postgresql/$PG_VERSION/main/postgresql.conf file.

2) running

pg_conftool /etc/postgresql/14/main/postgresql.conf set test_config on

3) checking that the end of the file contains

# test_config this is just a comment
test_config = on

for fixed versions of the package, instead of just

test_config = on

as it happens for affected versions.

[ Where problems could occur ]

While the changed regular expression is being tested during autopkgtest runs, the proposed patch may change its parsing behavior for unaccounted use cases. If this happens, we will need to cover such cases and propose further fixes.

[ Other info ]

We already submitted a fix to Debian in https://salsa.debian.org/postgresql/postgresql-common/-/merge_requests/17

Note that while lunar has one specific documentation line affected by this issue (as described below), the other affected series would only have this issue manifested for user specific crafted comments, AFAICT. This is why all SRUs for this bug are marked as low priority, differently from the lunar one.

[ Original report ]

While configuring Postgres for our Docker containers running Ubuntu/Postgres15 (new deployment) we have found a peculiar problem.

When setting the "logging_collector = on/off" we have found pg_conftool uncomments the incorrect line, please note this does not happen to any other line, but very well may as we do not change all default settings.

Here is an excerpt for /etc/postgresql/15/main/postgresql.conf referring to logging_collector:

Default install:
------------------------------------
446 # eventlog, depending on platform.
447 # csvlog and jsonlog require
448 # logging_collector to be on.
449
450 # This is used when logging to stderr:
451 #logging_collector = off # Enable capturing of stderr, jsonlog,
452 # and csvlog into log files. Required
453 # to be on for csvlogs and jsonlogs.
-------------------------------------

Please note this ONLY happens on PG15 which happens to have comments on line 447/448 in a separate line placing "logging_collector" in the start of the sentence.

When running:

-------------------------------------
# pg_conftool /etc/postgresql/15/main/postgresql.conf set logging_collector on
-------------------------------------

It will modify the postgresql.conf but not how you would think it would; after setting logging_collector to on:

-------------------------------------
447 # csvlog and jsonlog require
448 logging_collector on
449
450 # This is used when logging to stderr:
451 #logging_collector = off # Enable capturing of stderr, jsonlog,
452 # and csvlog into log files. Required
453 # to be on for csvlogs and jsonlogs.
-------------------------------------

The incorrect line was uncommented (448), it should have uncommented line 451.

Looking into the source code for pg_conftool we found the regex used for parsing postgresql.conf is the problem, it recognizes line 448 as a valid setting line when it is indeed a comment:

-------------------------------------
448 # logging_collector to be on
-------------------------------------

Here is the snip for file ".\postgresql-common\PgCommon.pm" which handles parsing:

-------------------------------------
my $found = 0;
    # first, search for an uncommented setting
    for (my $i=0; $i <= $#lines; ++$i) {
 if ($lines[$i] =~ /^\s*($key)(\s*(?:=|\s)\s*)\w+\b((?:\s*#.*)?)/i or
     $lines[$i] =~ /^\s*($key)(\s*(?:=|\s)\s*)'[^']*'((?:\s*#.*)?)/i) {
     $lines[$i] = "$1$2$value$3\n";
     $found = 1;
     last;
 }
    }

    # now check if the setting exists as a comment; if so, change that instead
    # of appending
    if (!$found) {
 for (my $i=0; $i <= $#lines; ++$i) {
     if ($lines[$i] =~ /^\s*#\s*($key)(\s*(?:=|\s)\s*)\w+\b((?:\s*#.*)?)/i or
  $lines[$i] =~ /^\s*#\s*($key)(\s*(?:=|\s)\s*)'[^']*'((?:\s*#.*)?)/i) {
  $lines[$i] = "$1$2$value$3\n";
  $found = 1;
  last;
     }
 }
-------------------------------------

See original description

Related branches

~athos-ribeiro/ubuntu/+source/postgresql-common:fix-set-conffile-regex-bionic
git-ubuntu bot: Approve
Bryce Harrington (community): Approve
Canonical Server Reporter: Pending requested
Diff: 107 lines (+26/-2)
3 files modified
PgCommon.pm (+2/-2)
debian/changelog (+6/-0)
t/005_PgCommon.t (+18/-0)
~athos-ribeiro/ubuntu/+source/postgresql-common:fix-set-conffile-regex-focal
git-ubuntu bot: Approve
Bryce Harrington (community): Approve
Canonical Server Reporter: Pending requested
Diff: 107 lines (+26/-2)
3 files modified
PgCommon.pm (+2/-2)
debian/changelog (+6/-0)
t/005_PgCommon.t (+18/-0)
~athos-ribeiro/ubuntu/+source/postgresql-common:fix-set-conffile-regex-jammy
git-ubuntu bot: Approve
Bryce Harrington (community): Approve
Canonical Server Reporter: Pending requested
Diff: 138 lines (+31/-5)
5 files modified
PgCommon.pm (+2/-2)
debian/changelog (+7/-0)
debian/control (+2/-1)
debian/rules (+2/-2)
t/005_PgCommon.t (+18/-0)
~athos-ribeiro/ubuntu/+source/postgresql-common:fix-set-conffile-regex-kinetic
git-ubuntu bot: Approve
Bryce Harrington (community): Approve
Canonical Server Reporter: Pending requested
Diff: 107 lines (+26/-2)
3 files modified
PgCommon.pm (+2/-2)
debian/changelog (+6/-0)
t/005_PgCommon.t (+18/-0)
~athos-ribeiro/ubuntu/+source/postgresql-common:fix-set-conffile-regex
git-ubuntu bot: Approve
Sergio Durigan Junior (community): Approve
Canonical Server Reporter: Pending requested
Diff: 121 lines (+28/-3)
4 files modified
PgCommon.pm (+2/-2)
debian/changelog (+6/-0)
debian/control (+2/-1)
t/005_PgCommon.t (+18/-0)
Revision history for this message
Bryce Harrington (bryce) wrote :

Thanks for the bug report and suggested fix.

I've confirmed the behavior occurs as described, in a lunar lxc container:

root@triage-lunar:/home/bryce# pg_conftool /etc/postgresql/15/main/postgresql.conf set logging_collector on
root@triage-lunar:/home/bryce# grep logging_collector /etc/postgresql/15/main/postgresql.conf
logging_collector on
#logging_collector = off # Enable capturing of stderr, jsonlog,
# These are only used if logging_collector is on:
root@triage-lunar:/home/bryce#

tags: added: server-todo
Changed in postgresql-common (Ubuntu):
importance: Undecided → High
status: New → Triaged
Athos Ribeiro (athos-ribeiro)
Changed in postgresql-common (Ubuntu):
assignee: nobody → Athos Ribeiro (athos-ribeiro)
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Hi wycitox incox,

Thanks again for reporting this one.

Did you experience any issues with this regex matching bug other than having comments removed? While the incorrect line was parsed, it seems that the issue here is that we end up erasing comments (which could, in a different situation, be a custom user comment).

Changed in postgresql-common (Ubuntu):
importance: High → Medium
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote (last edit ):

I submitted a fix to Debian in https://salsa.debian.org/postgresql/postgresql-common/-/merge_requests/17

summary: - pg_conftool does not "recognize" lines for logging_collector
+ pg_conftool mistakes regular comments for commented config options
Athos Ribeiro (athos-ribeiro)
description: updated
Changed in postgresql-common (Ubuntu Jammy):
status: New → Triaged
assignee: nobody → Athos Ribeiro (athos-ribeiro)
Athos Ribeiro (athos-ribeiro)
Changed in postgresql-common (Ubuntu Kinetic):
status: New → Triaged
Changed in postgresql-common (Ubuntu Focal):
status: New → Triaged
Changed in postgresql-common (Ubuntu Bionic):
status: New → Triaged
assignee: nobody → Athos Ribeiro (athos-ribeiro)
Changed in postgresql-common (Ubuntu Focal):
assignee: nobody → Athos Ribeiro (athos-ribeiro)
Changed in postgresql-common (Ubuntu Kinetic):
assignee: nobody → Athos Ribeiro (athos-ribeiro)
importance: Undecided → Low
Changed in postgresql-common (Ubuntu Jammy):
importance: Undecided → Low
Changed in postgresql-common (Ubuntu Focal):
importance: Undecided → Low
Changed in postgresql-common (Ubuntu Bionic):
importance: Undecided → Low
Athos Ribeiro (athos-ribeiro)
description: updated
Christian Ehrhardt  (paelzer)
Changed in postgresql-common (Ubuntu Lunar):
status: Triaged → Fix Released
Athos Ribeiro (athos-ribeiro)
tags: removed: server-todo
Revision history for this message
Steve Langasek (vorlon) wrote :

I do not think the severity of this bug (which has apparently been around for >5 years, given the list of affected releases) justifies an SRU on its own. I would be willing to accept this into -proposed but only with a block-proposed tag, such that it would only be released when another reason to SRU comes along. I would also not want to accept this into bionic, since bionic EOLs this month and another SRU in that timeframe is highly unlikely.

Please let me know if you would like to proceed along these lines, or if I should reject the SRUs instead.

Changed in postgresql-common (Ubuntu Kinetic):
status: Triaged → Incomplete
Changed in postgresql-common (Ubuntu Jammy):
status: Triaged → Incomplete
Changed in postgresql-common (Ubuntu Focal):
status: Triaged → Incomplete
Changed in postgresql-common (Ubuntu Bionic):
status: Triaged → Incomplete
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Without the fix for LP: #1973382, any future SRUs in jammy will introduce a regression due to changes in debhelper/debconf. So at least that change should be staged in jammy.

We could stage it at least in kinetic and jammy then (I will also add a block-proposed-jammy tag to LP: #1973382).

+1 on your bionic assessment. I'd also be OK with just dropping this one for focal.

tags: added: block-proposed-jammy block-proposed-kinetic
Changed in postgresql-common (Ubuntu Kinetic):
status: Incomplete → Triaged
Changed in postgresql-common (Ubuntu Jammy):
status: Incomplete → Triaged
Changed in postgresql-common (Ubuntu Focal):
status: Incomplete → Won't Fix
Changed in postgresql-common (Ubuntu Bionic):
status: Incomplete → Won't Fix
Revision history for this message
Steve Langasek (vorlon) wrote : Please test proposed package

Hello wycitox, or anyone else affected,

Accepted postgresql-common into kinetic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/postgresql-common/242ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-kinetic to verification-done-kinetic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-kinetic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in postgresql-common (Ubuntu Kinetic):
status: Triaged → Fix Committed
tags: added: verification-needed verification-needed-kinetic
Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (postgresql-common/242ubuntu1.1)

All autopkgtests for the newly accepted postgresql-common (242ubuntu1.1) for kinetic have finished running.
The following regressions have been reported in tests triggered by the package:

bacula/9.6.7-5 (armhf)
mediawiki/1:1.35.7-1 (armhf)
mediawiki-extension-codemirror/4.0.0~git20210902.a63f3c2-3 (armhf, ppc64el)
mediawiki-extension-youtube/1.9.3~git20200711.0f87a53-4 (armhf)
mediawiki-skin-greystuff/1.0.8~git20200711.479faf1-4 (armhf)
python-django-dbconn-retry/0.1.6-3 (arm64)
redmine/5.0.2-2 (armhf)
resource-agents/1:4.11.0-1ubuntu1 (amd64, armhf, s390x)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/kinetic/update_excuses.html#postgresql-common

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Robie Basak (racb) wrote :

Is there anyone who would actually benefit from this being fixed in an SRU?

I appreciate the report from @wycitox incox and I understand that this resulted in a fix in newer releases, so that was very helpful - thank you! However, I don't see any actual request for this to be fixed in 22.04, and for the use case presented as Docker containers, a typical Docker-based workflow doesn't involve anyone typically depending on comments in the output Docker image, right?

If there is someone who actually would benefit from this, then sure, let's SRU into the LTS. Please let us know! But I haven't seen anyone asking for this, and so why would we take any risk of regression at all in case the regexp change affects some other unexpected use case. Given that nobody else appears to have noticed this for years and years, isn't it sufficient to just fix it in the next release as already has been done?

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Thanks, Robie.

Would you rather reject the queued upload and close the jammy task as wontfix? I can then continue the discussions on LP: #1973382 and if we agree that the described behavior bug needs fixing there, I will prepare a new upload for that one.

Revision history for this message
Robie Basak (racb) wrote :

I wanted to give @wycitox incox the opportunity to tell us about their use case if they would actually benefit from this fix in 22.04. But yeah, if that's not the case, and nobody else appears to need it, then let's have an upload without this change. So I won't reject right now but feel free to upload over the top.

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

The kinetic test failures were all flaky tests and are all green now.

I also verified the package in -proposed acording to the [ test plan ] and the final config file is as expected (i.e., it no longer gets comments removed from pg_conftool).

tags: added: verification-done-kinetic
removed: verification-needed-kinetic
Revision history for this message
Steve Langasek (vorlon) wrote : Please test proposed package

Hello wycitox, or anyone else affected,

Accepted postgresql-common into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/postgresql-common/238ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in postgresql-common (Ubuntu Jammy):
status: Triaged → Fix Committed
tags: added: verification-needed-jammy
Revision history for this message
Steve Langasek (vorlon) wrote : Proposed package upload rejected

An upload of postgresql-common to focal-proposed has been rejected from the upload queue for the following reason: "rejected per uploader".

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (postgresql-common/238ubuntu0.1)

All autopkgtests for the newly accepted postgresql-common (238ubuntu0.1) for jammy have finished running.
The following regressions have been reported in tests triggered by the package:

resource-agents/1:4.7.0-1ubuntu7 (amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/jammy/update_excuses.html#postgresql-common

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

The regressions above were not related to the proposed changes. re-triggering some tests fixed the issue.

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

The fix works according to the test plan. User comments are no longer being removed by the postgresql-common tooling.

tags: added: verification-done verification-done-jammy
removed: verification-needed verification-needed-jammy
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.