GUC flags

On Sun, Nov 28, 2021 at 09:08:33PM -0600, Justin Pryzby wrote:

This isn't flagged with GUC_EXPLAIN:
enable_incremental_sort

Yeah, that's inconsistent.

This isn't marked GUC_NOT_IN_SAMPLE, like all other DEVELOPER_OPTIONS:
trace_recovery_messages

Indeed.

I'm not sure jit_tuple_deforming should be marked GUC_NOT_IN_SAMPLE.
I disable this one because it's slow for tables with many attributes.
Same for jit_expressions ?

That would be consistent. Both are not in postgresql.conf.sample.

bgwriter_lru_maxpages should have GUC_UNIT_BLOCKS

max_identifier_length should have BYTES (like log_parameter_max_length and
track_activity_query_size).

block_size and wal_block_size should say BYTES (like wal_segment_size)
And all three descriptions should say ".. in [prefix]bytes" (but see below).

Okay for these.

Maybe these should have COMPAT_OPTIONS_PREVIOUS:
integer_datetimes
ssl_renegotiation_limit

Hmm. Okay as well for integer_datetimes.

autovacuum_freeze_max_age has a comment about pg_resetwal which is obsolete
since 74cf7d46a.

checkpoint_warning refers to "checkpoint segments", which is obsolete since
88e982302.

That's part of 0002. That's a bit weird to use now, so I'd agree with
your suggestion to use "WAL segments" instead.

0001, to adjust the units, and 0003, to make the GUC descriptions less
unit-dependent, are good ideas.

-      gettext_noop("Use of huge pages on Linux or Windows."),
+      gettext_noop("Enable use of huge pages on Linux or Windows."),
This should be "Enables use of".

    {"compute_query_id", PGC_SUSET, STATS_MONITORING,
-    gettext_noop("Compute query identifiers."),
+    gettext_noop("Enables in-core computation of a query identifier."),
This could just be "Computes"?

I am not completely sure that all the contents of 0002 are
improvements, but the suggestions done for huge_pages,
ssl_passphrase_command_supports_reload, checkpoint_warning and
commit_siblings seem fine to me.
--
Michael

On Mon, Nov 29, 2021 at 05:04:01PM +0900, Michael Paquier wrote:

0001, to adjust the units, and 0003, to make the GUC descriptions less
unit-dependent, are good ideas.

Actually, after sleeping on it and doing some digging in
codesearch.debian.org, changing the units of max_identifier_length,
block_size and wal_block_size could induce some breakages for anything
using a SHOW command, something that becomes particularly worse now
that SHOW is supported in replication connections, and it would force
clients to know and parse the units of a value. Perhaps I am being
too careful here, but that could harm a lot of users. It is worth
noting that I have found some driver code making use of pg_settings,
which would not be influenced by such a change, but it is unsafe to
assume that everybody does that.

The addition of GUC_EXPLAIN for enable_incremental_sort, the comment
fix for autovacuum_freeze_max_age, the use of COMPAT_OPTIONS_PREVIOUS
for ssl_renegotiation_limit and the addition of GUC_NOT_IN_SAMPLE for
trace_recovery_messages are fine, though.

I am not completely sure that all the contents of 0002 are
improvements, but the suggestions done for huge_pages,
ssl_passphrase_command_supports_reload, checkpoint_warning and
commit_siblings seem fine to me.

Hmm, I think the patched description of checkpoint_warning is not that
much an improvement compared to the current one. While the current
description uses the term "checkpoint segments", which is, I agree,
weird. The new one would lose the term "checkpoint", making the short
description of the parameter lose some of its context.

I have done a full review of the patch set, and applied the obvious
fixes/improvements as of be54551. Attached is an extra patch based on
the contents of the whole set sent upthread:
- Improvement of the description of checkpoint_segments.
- Reworded the description of all parameters using "N units", rather
than just switching to "this period of units". I have been using
something more generic.

Thoughts?
--
Michael

On Tue, Nov 30, 2021 at 03:36:45PM +0900, Michael Paquier wrote:

-			gettext_noop("Forces a switch to the next WAL file if a "
-						 "new file has not been started within N seconds."),
+			gettext_noop("Sets the amount of time to wait before forcing a "
+						 "switch to the next WAL file."),

..

-			gettext_noop("Waits N seconds on connection startup after authentication."),
+			gettext_noop("Sets the amount of seconds to wait on connection "
+						 "startup after authentication."),

"amount of time", like above.

-			gettext_noop("Waits N seconds on connection startup before authentication."),
+			gettext_noop("Sets the amount of seconds to wait on connection "
+						 "startup before authentication."),

same

{
{"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
-			gettext_noop("Enables warnings if checkpoint segments are filled more "
-						 "frequently than this."),
+			gettext_noop("Sets the maximum time before warning if checkpoints "
+						 "triggered by WAL volume happen too frequently."),
gettext_noop("Write a message to the server log if checkpoints "
-						 "caused by the filling of checkpoint segment files happens more "
+						 "caused by the filling of WAL segment files happens more "

It should say "happen" , since it's referring to "checkpoints".
That was a pre-existing issue.

{"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
-			gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
+			gettext_noop("Sets the maximum amount of data logged for bind "
+						 "parameter values when logging statements."),

I think this one should actually say "in bytes" or at least say "maximum
length". It seems unlikely that someone is going to specify this in other
units, and it's confusing to everyone else to refer to "amount of data" instead
of "length in bytes".

{"log_parameter_max_length_on_error", PGC_USERSET, LOGGING_WHAT,
-			gettext_noop("When reporting an error, limit logged parameter values to first N bytes."),
+			gettext_noop("Sets the maximum amount of data logged for bind "
+						 "parameter values when logging statements, on error."),

same

-			gettext_noop("Automatic log file rotation will occur after N minutes."),
+			gettext_noop("Sets the maximum amount of time to wait before "
+						 "forcing log file rotation."),

Should it say "maximum" ? Does that mean anything ?

--
Justin

On Wed, Dec 01, 2021 at 01:59:05AM -0600, Justin Pryzby wrote:

On Tue, Nov 30, 2021 at 03:36:45PM +0900, Michael Paquier wrote:

-			gettext_noop("Waits N seconds on connection startup before authentication."),
+			gettext_noop("Sets the amount of seconds to wait on connection "
+						 "startup before authentication."),

same

Thanks. This makes things more consistent.

{
{"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
-			gettext_noop("Enables warnings if checkpoint segments are filled more "
-						 "frequently than this."),
+			gettext_noop("Sets the maximum time before warning if checkpoints "
+						 "triggered by WAL volume happen too frequently."),
gettext_noop("Write a message to the server log if checkpoints "
-						 "caused by the filling of checkpoint segment files happens more "
+						 "caused by the filling of WAL segment files happens more "

It should say "happen" , since it's referring to "checkpoints".
That was a pre-existing issue.

Indeed.

{"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
-			gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
+			gettext_noop("Sets the maximum amount of data logged for bind "
+						 "parameter values when logging statements."),

I think this one should actually say "in bytes" or at least say "maximum
length". It seems unlikely that someone is going to specify this in other
units, and it's confusing to everyone else to refer to "amount of data" instead
of "length in bytes".

Okay. Do you like the updated version attached?

-			gettext_noop("Automatic log file rotation will occur after N minutes."),
+			gettext_noop("Sets the maximum amount of time to wait before "
+						 "forcing log file rotation."),

Should it say "maximum" ? Does that mean anything ?

To be consistent with the rest of your suggestions, we could use here:
"Sets the amount of time to wait before forcing log file rotation"

Thanks,
--
Michael

@@ -2142,7 +2142,8 @@ static struct config_int ConfigureNamesInt[] =
{"post_auth_delay", PGC_BACKEND, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup after authentication."),
+			gettext_noop("Sets the amount of time to wait on connection "
+						 "startup after authentication."),

@@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
{"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup before authentication."),
+			gettext_noop("Sets the amount of time to wait on connection "
+						 "startup before authentication."),
gettext_noop("This allows attaching a debugger to the process."),

I wonder if these should say "Sets the amount of time to wait [before]
authentication during connection startup"

{"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
-			gettext_noop("Enables warnings if checkpoint segments are filled more "
-						 "frequently than this."),
+			gettext_noop("Sets the maximum time before warning if checkpoints "
+						 "triggered by WAL volume happen too frequently."),
gettext_noop("Write a message to the server log if checkpoints "
-						 "caused by the filling of checkpoint segment files happens more "
+						 "caused by the filling of WAL segment files happen more "
"frequently than this number of seconds. Zero turns off the warning."),

Should this still say "seconds" ?
Or change it to "this amount of time"?
I'm not sure.

{"log_rotation_age", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Automatic log file rotation will occur after N minutes."),
+			gettext_noop("Sets the amount of time to wait before forcing "
+						 "log file rotation."),
NULL,
GUC_UNIT_MIN
},
@@ -3154,7 +3159,8 @@ static struct config_int ConfigureNamesInt[] =

{
{"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Automatic log file rotation will occur after N kilobytes."),
+			gettext_noop("Sets the maximum size of log file to reach before "
+						 "forcing log file rotation."),

Actually, I think that for log_rotation_size, it should not say "forcing".

"Sets the maximum size a log file can reach before being rotated"

BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.

--
Justin

On Wed, Dec 01, 2021 at 09:34:39PM -0600, Justin Pryzby wrote:

@@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
{"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup before authentication."),
+			gettext_noop("Sets the amount of time to wait on connection "
+						 "startup before authentication."),
gettext_noop("This allows attaching a debugger to the process."),

I wonder if these should say "Sets the amount of time to wait [before]
authentication during connection startup"

Hmm. I don't see much a difference between both of wordings in this
context.

gettext_noop("Write a message to the server log if checkpoints "
-						 "caused by the filling of checkpoint segment files happens more "
+						 "caused by the filling of WAL segment files happen more "
"frequently than this number of seconds. Zero turns off the warning."),

Should this still say "seconds" ?
Or change it to "this amount of time"?
I'm not sure.

Either way would be fine by me, though I'd agree to be consistent and
use "this amount of time" here.

{"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Automatic log file rotation will occur after N kilobytes."),
+			gettext_noop("Sets the maximum size of log file to reach before "
+						 "forcing log file rotation."),

Actually, I think that for log_rotation_size, it should not say "forcing".

"Sets the maximum size a log file can reach before being rotated"

Okay. Fine by me.

BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.

This could cause small diffs in EXPLAIN outputs, which could be
surprising. This is not worth taking any risks.
--
Michael

On Thu, Dec 02, 2021 at 02:11:38PM +0900, Michael Paquier wrote:

On Wed, Dec 01, 2021 at 09:34:39PM -0600, Justin Pryzby wrote:

@@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
{"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
-			gettext_noop("Waits N seconds on connection startup before authentication."),
+			gettext_noop("Sets the amount of time to wait on connection "
+						 "startup before authentication."),
gettext_noop("This allows attaching a debugger to the process."),

I wonder if these should say "Sets the amount of time to wait [before]
authentication during connection startup"

Hmm. I don't see much a difference between both of wordings in this
context.

I find it easier to read "wait before authentication ..." than "wait ... before
authentication".

BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.

This could cause small diffs in EXPLAIN outputs, which could be
surprising. This is not worth taking any risks.

Only if one specifies explain(SETTINGS).
It's fine either way ;)

--
Justin

On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:

I find it easier to read "wait before authentication ..." than "wait ... before
authentication".

I have a hard time seeing a strong difference here. At the end, I
have used what you suggested, adjusted the rest based on your set of
comments, and applied the patch.
--
Michael

On Fri, Dec 03, 2021 at 10:06:47AM +0900, Michael Paquier wrote:

On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:

I find it easier to read "wait before authentication ..." than "wait ... before
authentication".

I have a hard time seeing a strong difference here. At the end, I
have used what you suggested, adjusted the rest based on your set of
comments, and applied the patch.

Thanks. One more item. The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions. This is not
useful.

$ (cd ./src/backend/utils/misc/; ./check_guc) |wc -l
68

With the attached:

$ (cd ./src/backend/utils/misc/; ./check_guc)
config_file seems to be missing from postgresql.conf.sample

That has a defacto exception for the "include" directive, which seems
reasonable.

This requires GNU awk. I'm not sure if that's a limitation of any
significance.

--
Justin

On Sun, Dec 05, 2021 at 11:38:05PM -0600, Justin Pryzby wrote:

Thanks. One more item. The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions. This is not
useful.

Indeed. Hmm. This script does a couple of things:
1) Check the format of the options defined in the various lists of
guc.c, which is something people format well, and pgindent also does
a part of this job.
2) Check that options in the hardcoded list of GUCs in
INTENTIONALLY_NOT_INCLUDED are not included in
postgresql.conf.sample
3) Check that nothing considered as a parameter in
postgresql.conf.sample is listed in guc.c.

Your patch removes 1) and 2), but keeps 3) to check for dead
parameter references in postgresql.conf.sample.

Is check_guc actually run on a periodic basis by somebody? Based on
the amount of false positives that has accumulated over the years, and
what `git grep` can already do for 3), it seems to me that we have
more arguments in favor of just removing it entirely.
--
Michael

On Mon, Dec 06, 2021 at 03:58:39PM +0900, Michael Paquier wrote:

On Sun, Dec 05, 2021 at 11:38:05PM -0600, Justin Pryzby wrote:

Thanks. One more item. The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions. This is not
useful.

Indeed. Hmm. This script does a couple of things:
1) Check the format of the options defined in the various lists of
guc.c, which is something people format well, and pgindent also does
a part of this job.
2) Check that options in the hardcoded list of GUCs in
INTENTIONALLY_NOT_INCLUDED are not included in
postgresql.conf.sample
3) Check that nothing considered as a parameter in
postgresql.conf.sample is listed in guc.c.

Your patch removes 1) and 2), but keeps 3) to check for dead
parameter references in postgresql.conf.sample.

The script checks that guc.c and sample config are consistent.

I think your undertanding of INTENTIONALLY_NOT_INCLUDED is not right.
That's a list of stuff it "avoids reporting" as an suspected error, not an
additional list of stuff to checks. INTENTIONALLY_NOT_INCLUDED is a list of
stuff like NOT_IN_SAMPLE, which is better done by parsing /NOT_IN_SAMPLE/.

Is check_guc actually run on a periodic basis by somebody? Based on
the amount of false positives that has accumulated over the years, and
what `git grep` can already do for 3), it seems to me that we have
more arguments in favor of just removing it entirely.

I saw that Tom updated it within the last 12 months, which I took to mean that
it was still being maintained. But I'm okay with removing it.

--
Justin

On Mon, Dec 06, 2021 at 07:36:55AM -0600, Justin Pryzby wrote:

The script checks that guc.c and sample config are consistent.

I think your undertanding of INTENTIONALLY_NOT_INCLUDED is not right.
That's a list of stuff it "avoids reporting" as an suspected error, not an
additional list of stuff to checks. INTENTIONALLY_NOT_INCLUDED is a list of
stuff like NOT_IN_SAMPLE, which is better done by parsing /NOT_IN_SAMPLE/.

Indeed. I got that wrong, thanks for clarifying.

I saw that Tom updated it within the last 12 months, which I took to mean that
it was still being maintained. But I'm okay with removing it.

Yes, I saw that as of bf8a662. With 42 incorrect reports, I still see
more evidence with removing it. Before doing anything, let's wait for
and gather some opinions. I am adding Bruce (as the original author)
and Tom in CC as they are the ones who have updated this script the
most in the last ~15 years.
--
Michael

On 08.12.21 07:27, Michael Paquier wrote:

I saw that Tom updated it within the last 12 months, which I took to mean that
it was still being maintained. But I'm okay with removing it.

Yes, I saw that as of bf8a662. With 42 incorrect reports, I still see
more evidence with removing it. Before doing anything, let's wait for
and gather some opinions. I am adding Bruce (as the original author)
and Tom in CC as they are the ones who have updated this script the
most in the last ~15 years.

I wasn't really aware of this script either. But I think it's a good
idea to have it. But only if it's run automatically as part of a test
suite run.

On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:

I wasn't really aware of this script either. But I think it's a good idea
to have it. But only if it's run automatically as part of a test suite run.

Okay. If we do that, I am wondering whether it would be better to
rewrite this script in perl then, so as there is no need to worry
about the compatibility of grep. And also, it would make sense to
return a non-zero exit code if an incompatibility is found for the
automation part.
--
Michael

On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:

On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:

I wasn't really aware of this script either. But I think it's a good idea
to have it. But only if it's run automatically as part of a test suite run.

Okay. If we do that, I am wondering whether it would be better to
rewrite this script in perl then, so as there is no need to worry
about the compatibility of grep. And also, it would make sense to
return a non-zero exit code if an incompatibility is found for the
automation part.

One option is to expose the GUC flags in pg_settings, so this can all be done
in SQL regression tests.

Maybe the flags should be text strings, so it's a nicer user-facing interface.
But then the field would be pretty wide, even though we're only adding it for
regression tests. The only other alternative I can think of is to make a
sql-callable function like pg_get_guc_flags(text guc).

On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote:

On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:

On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:

I wasn't really aware of this script either. But I think it's a good idea
to have it. But only if it's run automatically as part of a test suite run.

Okay. If we do that, I am wondering whether it would be better to
rewrite this script in perl then, so as there is no need to worry
about the compatibility of grep. And also, it would make sense to
return a non-zero exit code if an incompatibility is found for the
automation part.

One option is to expose the GUC flags in pg_settings, so this can all be done
in SQL regression tests.

Maybe the flags should be text strings, so it's a nicer user-facing interface.
But then the field would be pretty wide, even though we're only adding it for
regression tests. The only other alternative I can think of is to make a
sql-callable function like pg_get_guc_flags(text guc).

Fixed regression tests caused by another patches.

On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote:

On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:

On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:

I wasn't really aware of this script either. But I think it's a good idea
to have it. But only if it's run automatically as part of a test suite run.

Okay. If we do that, I am wondering whether it would be better to
rewrite this script in perl then, so as there is no need to worry
about the compatibility of grep. And also, it would make sense to
return a non-zero exit code if an incompatibility is found for the
automation part.

One option is to expose the GUC flags in pg_settings, so this can all be done
in SQL regression tests.

Maybe the flags should be text strings, so it's a nicer user-facing interface.
But then the field would be pretty wide, even though we're only adding it for
regression tests. The only other alternative I can think of is to make a
sql-callable function like pg_get_guc_flags(text guc).

Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207.

And added 0003, which changes to instead exposes the flags as a function, to
avoid changing pg_settings and exposing internally-defined integer flags in
that somewhat prominent view.

--
Justin

At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby <pryzby@telsasoft.com> wrote in

On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote:

On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:
One option is to expose the GUC flags in pg_settings, so this can all be done
in SQL regression tests.

Maybe the flags should be text strings, so it's a nicer user-facing interface.
But then the field would be pretty wide, even though we're only adding it for
regression tests. The only other alternative I can think of is to make a
sql-callable function like pg_get_guc_flags(text guc).

Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207.

And added 0003, which changes to instead exposes the flags as a function, to
avoid changing pg_settings and exposing internally-defined integer flags in
that somewhat prominent view.

Just an idea but couldn't we use flags in a series of one-letter flag
representations? It is more user-friendly than integers but shorter
than full-text representation.

+SELECT name, flags FROM pg_settings;
name | flags
------------------------+--------
application_name | ARsec
transaction_deferrable | Arsec
transaction_isolation | Arsec
transaction_read_only | Arsec

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jan 05, 2022 at 11:47:57AM +0900, Kyotaro Horiguchi wrote:

At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby <pryzby@telsasoft.com> wrote in

On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote:

On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:
One option is to expose the GUC flags in pg_settings, so this can all be done
in SQL regression tests.

Maybe the flags should be text strings, so it's a nicer user-facing interface.
But then the field would be pretty wide, even though we're only adding it for
regression tests. The only other alternative I can think of is to make a
sql-callable function like pg_get_guc_flags(text guc).

Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207.

And added 0003, which changes to instead exposes the flags as a function, to
avoid changing pg_settings and exposing internally-defined integer flags in
that somewhat prominent view.

Just an idea but couldn't we use flags in a series of one-letter flag
representations? It is more user-friendly than integers but shorter
than full-text representation.

+SELECT name, flags FROM pg_settings;
name | flags
------------------------+--------
application_name | ARsec
transaction_deferrable | Arsec
transaction_isolation | Arsec
transaction_read_only | Arsec

It's a good idea.

I suppose you intend that "A" means it's enabled and "a" means it's disabled ?

A => show all
R => reset all
S => not in sample
E => explain
C => computed

Which is enough to support the tests that I came up with:

+       (flags&4) != 0 AS no_show_all,
+       (flags&8) != 0 AS no_reset_all,
+       (flags&32) != 0 AS not_in_sample,
+       (flags&1048576) != 0 AS guc_explain,
+       (flags&2097152) != 0 AS guc_computed

However, I think if we add a field to pg_stat_activity, it would be in a
separate patch, expected to be independently useful.

1) expose GUC flags to pg_stat_activity;
2) rewrite check_guc as a sql regression test;

In that case, *all* the flags should be exposed. There's currently 20, which
means it may not work well after all - it's already too long, and could get
longer, and/or overflow the alphabet...

I think pg_get_guc_flags() may be best, but I'm interested to hear other
opinions.

--
Justin