Re: [RFC] Removing "magic" oids

Hi,

On 2019-07-07 10:00:35 -0700, Noah Misch wrote:
> On Tue, Nov 20, 2018 at 01:20:04AM -0800, Andres Freund wrote:
> > On 2018-11-14 21:02:41 -0800, Andres Freund wrote:
> > > > The point of the test is to exercise OidGenLock by issuing many parallel
> > > > GetNewOidWithIndex() and verifying absence of duplicates.  There's nothing
> > > > special about OidGenLock, but it is important to use an operation that takes a
> > > > particular LWLock many times, quickly.  If the test query spends too much time
> > > > on things other than taking locks, it will catch locking races too rarely.
> > > 
> > > Sequences ought to do that, too. And if it's borked, we'd hopefully see
> > > unique violations. But it's definitely not a 1:1 replacement.
> 
> > I've tested this on ppc.  Neither the old version nor the new version
> > stress test spinlocks sufficiently to error out with weakened spinlocks
> > (not that surprising, there are no spinlocks in any hot path of either
> > workload). Both versions very reliably trigger on weakened lwlocks. So I
> > think we're comparatively good on that front.
> 
> I tested this on xlc, the compiler that motivated the OID test, and the v12+
> version of the test didn't catch the bug[1] with xlc 13.1.3.  CREATE TYPE
> ... AS ENUM generates an OID for each label, so the attached patch makes the
> v12+ test have locking behavior similar to its v11 ancestor.

Interesting.  It's not obvious to me why that works meaningfully better.


> [1] https://postgr.es/m/flat/a72cfcb0-37d0-de2f-b3ec-f38ad8d6a8cc@postgrespro.ru

> diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
> index dc2c72f..3b097a9 100644
> --- a/src/bin/pgbench/t/001_pgbench_with_server.pl
> +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
> @@ -58,27 +58,20 @@ sub pgbench
>  	return;
>  }
>  
> -# Test concurrent insertion into table with serial column.  This
> -# indirectly exercises LWLock and spinlock concurrency.  This test
> -# makes a 5-MiB table.
> -
> -$node->safe_psql('postgres',
> -	'CREATE UNLOGGED TABLE insert_tbl (id serial primary key); ');
> -
> +# Test concurrent OID generation via pg_enum_oid_index.  This indirectly
> +# exercises LWLock and spinlock concurrency.
> +my $labels = join ',', map { "'l$_'" } 1 .. 1000;
>  pgbench(
>  	'--no-vacuum --client=5 --protocol=prepared --transactions=25',
>  	0,
>  	[qr{processed: 125/125}],
>  	[qr{^$}],
> -	'concurrent insert workload',
> +	'concurrent OID generation',
>  	{
>  		'001_pgbench_concurrent_insert' =>
> -		  'INSERT INTO insert_tbl SELECT FROM generate_series(1,1000);'
> +		  "CREATE TYPE pg_temp.e AS ENUM ($labels); DROP TYPE pg_temp.e;"
>  	});

Hm, perhaps we should just do something stupid an insert into a catalog
table, determining the oid to insert with pg_nextoid? That ought to be a
lot faster and thus more "stress testing" than going through a full
blown DDL statement?  But perhaps that's just too ugly.

Greetings,

Andres Freund