Allow empty string value for config entries

[antaflos]

Summary

PostgreSQL supports and allows config entries, such as those in postgresql.conf, to be set to the empty string. The postgresql::server::config_entry defined type, however, requires String[1] when supplying string values. This doesn't allow for the empty string.

This change relaxes the allowed data types for the value parameter of postgresql::server::config_entry to String from String[1] and adds a spec test to support the change.

Related Issues (if any)

#1602

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified. (For example puppet apply)

[antaflos]

I don't think I can do anything about the failing tests (labeller and mend). The rest looks good.

[bastelfreak]

@antaflos can you please document in the spec test some examples of options where this makes sense? And please rebase after #1599 got merged.

[antaflos]

@bastelfreak Will do. Our use case for empty strings here is that we have applications that use transaction-bound session variables, as in SET LOCAL awsome_app.auth_user_id = 123456 and SET LOCAL awesome_app.tenant_id = 893745. This pertains to auditability and row-level security.

PostgreSQL 13 (and possibly newer versions also) requires such session variables to be defined beforehand and initialised with a default value, such as the empty string, in postgresql.conf.

[antaflos]

So this has been open for a long while now. Any chance to get it merged?

[antaflos]

This is still open, even though approvals were given. What do I need to do to get this merged?

Some acceptance tests are failing, but apparently not because of my changes.

[ekohl]

I wondered about purging empty entries, but that's done with Undef:

puppetlabs-postgresql/manifests/server.pp

Lines 220 to 225 in d5911ec

	$config_entries.each |$entry, $value| {
	postgresql::server::config_entry { $entry:
	ensure => bool2str($value =~ Undef, 'absent', 'present'),
	value => $value,
	}
	}

[ekohl]

Should the array also allow empty strings?

[antaflos]

I am not aware of a use-case where an array would contain empty string values.

But I am not even sure if array values make sense for Postgres config entries, because it seems to use just (quoted) strings, integers, floats and booleans (https://www.postgresql.org/docs/current/config-setting.html#CONFIG-SETTING-NAMES-VALUES). And from quickly looking at the postgresql_conf provider I don't immediately see where/how it even handles array values? But in any case, I don't think we need to touch this part of the functionality here. Just support for single, empty string values would be nice :)

[ekohl]

That may actually be a regression introduced in 179472b. It was added in 36674b3 but contain any description. #1434 does point to listen_addresses and now I wonder if that functionality actually works now.

[antaflos]

Indeed, it seems the accepted data types of $value were changed to include Array[String[1]], and the "parsed" provider was replaced by a "ruby" provider, but there doesn't seem to have been any code added to actually handle such array values. The parsed provider had something like h[:value].join(', ') when h[:value] was an array, but I see no equivalent in the ruby provider.

But I suppose this discussion is best moved elsewhere, probably to its own bug report?

[ekohl]

Indeed, it seems the accepted data types of $value were changed to include Array[String[1]], and the "parsed" provider was replaced by a "ruby" provider, but there doesn't seem to have been any code added to actually handle such array values. The parsed provider had something like h[:value].join(', ') when h[:value] was an array, but I see no equivalent in the ruby provider.

If the implementation is h[:value].join(', ') then an empty array should result in an empty string and allowing empty strings within the array doesn't make sense. It'd result in , which is probably never valid. In other words, consider my comments addressed.

But I suppose this discussion is best moved elsewhere, probably to its own bug report?

Yes, agreed.

[antaflos]

@ekohl Thank you for taking the time to look at and review this!

[ekohl]

CI is busted because Perforce is still using ubuntu-20.04. I think we should wait until they fix that