Re: [PG19][PATCH] Make postgres_fdw statistics import atomic - Mailing list pgsql-hackers

So this situation happens when a remote table has pg_stats statistics, but has no underlying data to sample. There are two ways that can happen:

1) The table was populated, analyzed, and then the rows were removed via delete/truncate.

2) The table was not populated since last analysis or creation, but someone used statistics import functions on it.

 

Text field on source table.

 

 

Integer field on destination table. Which means that if 'bad-x' was a potential value that could have been sent over the wire for processing by the FDW table and we did a regular query:

    # update remote_t set a = 111, b = 'bad-x';
    UPDATE 2
    # SELECT * FROM ft;
    ERROR:  invalid input syntax for type integer: "bad-x"
    CONTEXT:  column "b" of foreign table "ft"

So already we're dealing with a very brittle situation. Not only is there a type mismatch in the "b" columns that can cause ordinary queries to fail, but we've shown that situation can and does happen.

As is, this will fail in production until someone remote comes along and deletes the bad-x row, and the situation can re-occur until someone local redefines or removes the "b" column.

 

So how would a table that didn't have stats import have handled this?

    # create foreign table ft2 (a int, b int) server loopback options (table_name 'remote_t');
    CREATE FOREIGN TABLE
    # analyze ft2;
    ERROR:  invalid input syntax for type integer: "bad-x"
    CONTEXT:  column "b" of foreign table "ft2"

And how would the first foreign table handle this if we fixed the records in a way that didn't also leave the table empty?

    # UPDATE remote_t SET b = 4;
    UPDATE 2
    # analyze ft;
    WARNING:  invalid input syntax for type integer: "bad-x"
    WARNING:  could not import statistics for foreign table "public.ft" --- attribute statistics import failed for column "b" of this foreign table
    ANALYZE

    # SELECT attname, most_common_vals FROM pg_stats WHERE tablename = 'ft';
     attname | most_common_vals
    ---------+------------------
     a       | {111}
     b       | {4}
    (2 rows)

So fallback to sampling works as intended, but not if the remote table was empty.

So this problem occurs only when a foreign table is mis-configured in such a way as to create a conversion error on the inputs from the remote table's stats, this went undiscovered (i.e. nobody queried the foreign table) before the foreign table was analyzed, AND the remote table has been emptied since it was analyzed but not yet re-analyzed at the time when the remote table was analyzed.

And the negative consequence of this is that a query plan will assume that the table contains rows when it actually does not. That seems like a problem we already have in this situation.

    CREATE TABLE ihaverows (x int);

    INSERT INTO ihaverows SELECT g.g FROM generate_series(1,100000) AS g;

    ANALYZE ihaverows;

    DELETE FROM ihaverows;

    SELECT relname, relpages, reltuples FROM pg_class where relname = 'ihaverows';

      relname  | relpages | reltuples
    -----------+----------+-----------
     ihaverows |      443 |    100000
    (1 row)

So the impact is that until the remote table is re-analyzed and then the foreign table is re-analyzed, we get over-estimates on an empty table. In a real-world situation that table would be repopulated and reanalyzed fairly quickly to restore business operations, and the bad stats issue corrects itself, exactly as would happen if we truncated and reloaded a local table. I'd call that low-risk, low-impact.

Furthermore, It's pretty reasonable to assume that the table will be re-loaded with data shortly, in which case the "bad" stats and "bad" row estimate would then be more accurate for the table which is populated but not yet re-analyzed than would stale stats showing a table to be empty when it actually has lots of rows. I mention this because this is the exact scenario that caused me to want to create statistics import functions in the first place: A partitioned table is being queried frequently/continuously by an application. An ETL is loading millions of rows into a partition that was empty just minutes ago. The stats for the partition reported it as empty, so the query planner said because the partition was empty, a table scan was the cheapest plan....oops. In this situation (on an Oracle database) we determined that injecting the stats from a populated partition into the empty partiion, while all wrong on histograms, gave a better row estimate than the collected stats (0 rows) and we knew the ETL would re-analyze the partition when it was done.

As for the C patch itself, it seems overengineered. It would be easier to enhance the failure case when calling import_attribute_statistics to make a note of where it was in the attribute loop, then walk backward calling delete_pg_statistic() on the attributes that we had set before the failure. But we shouldn't need to do that, as do_analyze_rel() should already handle cleaning up pg_statistics rows for a table that used to have rows but now doesn't....so I'd need to dig more to determine if this corner case only happens if the foreign table had never had a successful analyze before the bad data was inserted, but this dive is probably already deeper than most people wanted, so I'm going to take a decompression stop here.

Summary: I can't see how this would happen at all in a foreign table that correctly represented the remote data. Even then, it only happens if the table is analyzed in a specific middle step of remediating the remote data, and during that time the impact is low (no data loss, no incorrect queries), and would resolve itself after the expected remediation. If this needs a fix at all, it's probably not unique to postgres_fdw, so I speculate that we'd want to teach do_analyze_rel() to know that a table that analyzed as empty, but had an attempted remote stats fetch will need to be zeroed out even if it already looks zeroed out.