Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Índices no Banco de Dados #1156

Closed
filipedeschamps opened this issue Dec 19, 2022 · 14 comments
Closed

Índices no Banco de Dados #1156

filipedeschamps opened this issue Dec 19, 2022 · 14 comments
Labels
back Envolve modificações no backend desempenho Melhoria de desempenho

Comments

@filipedeschamps
Copy link
Owner

Contexto

Antes do lançamento, se eu não me engano o TabNews não tinha índices voltados a performance (ele tinha apenas voltado para o uniqueness de um conteúdo), e foi muito interessante ver o comportamento disto em produção 😅 O gráfico abaixo é o load do Postgres no dia do lançamento:

image

No dia seguinte, o problema de processamento estava acontecendo de novo, e então eu apliquei em produção a segunda parte desse PR que trazia índice para o balance_operation:

image

O que fez o load do gráfico li de cima voltar a ficar saudável, literalmente foi aplicar isso aqui abaixo:

exports.up = async (pgm) => {
  await pgm.createIndex('balance_operations', ['balance_type', 'recipient_id']);
};

É um índice de múltipla coluna entre balance_type e recipient_id por conta disso ser usado na procedure get_current_balance:

await pgm.createFunction(
'get_current_balance',
[
{
name: 'balance_type_input',
mode: 'IN',
type: 'text',
},
{
name: 'recipient_id_input',
mode: 'IN',
type: 'uuid',
},
],
{
returns: 'integer',
language: 'plpgsql',
replace: true,
},
`
DECLARE
total_balance integer;
BEGIN
total_balance := (
SELECT
COALESCE(sum(amount), 0)
FROM
balance_operations
WHERE
balance_type = balance_type_input
AND recipient_id = recipient_id_input
);
RETURN total_balance;
END;
`
);
};

Mais especificamente nesta parte:

WHERE
balance_type = balance_type_input
AND recipient_id = recipient_id_input

Então qualquer WHERE que queira aproveitar este cache, precisa pedir os dados nesta exata mesma sequência.

Bom, escrevi tudo isto para nos animar a procurar mais oportunidades de criar índices, pois tem em todos os lugares do TabNews, desde criação de usuário, criação de conteúdo, leitura de conteúdo, verificação de sessão ativa, literalmente tudo tem margem para otimização.

Execução

Eu sou zero especialista em criar índices, mas devemos tomar cuidado para não criar índices que não tragam resultados reais e para isto sugiro utilizar ferramentas auxiliares para comprovar que, o que estamos fazendo, está de fato trazendo resultado, e tudo isto pode ser testado com a instância local do Postgres que rodamos com o Docker. As ferramentas auxiliares que coletei foram a seguinte, mas não deixem de complementar com outras sugestões:

Sugiro também tomar cuidado com índices de múltiplas colunas, pois pelo que estudei, as vezes o Postgres consegue se dar muito bem combinando vários índices separados, e isto é ótimo, pois nos dá mais flexibilidade nas queries.

Fora isso, para certas coisas precisaremos rearquitetar a modelagem, como por exemplo, para o findChildrenTree() do content, vamos provavelmente adicionar uma coluna root_id e indexar tudo por ali.

Bom, qualquer novo PR que for merged sobre índice, vamos listar ele aqui nessa seção de Execução, então por hora vou listar somente o que já foi feito:

@FabricioFFC
Copy link
Contributor

@filipedeschamps e @aprendendofelipe vcs têm habilitado em produção o slow query log? Ele é uma boa fonte de informações, para sabermos melhor quais queries precisam ser otimizadas, se tiverem, podem disponibilizar por favor?

Caso precisem de ajudar para habilitar, a referência que linkei explica e posso ajudar também. Essa é uma otimização que já fiz no passado e gostaria de ajudar aqui :)

@filipedeschamps
Copy link
Owner Author

@FabricioFFC muito massa te ver aqui no repositório 😍 para quem não sabe eu e o Fabrício já trabalhamos juntos no passado 🤝

E não está habilitado, o único parameter group que temos habilitado é este:

parameter {
name = "idle_session_timeout"
value = "0"
}

@FabricioFFC
Copy link
Contributor

@filipedeschamps @aprendendofelipe show! abri uma PR habilitando, qq coisa só me avisar.

@filipedeschamps
Copy link
Owner Author

@FabricioFFC segue os principais logs:

duration: 2619.477 ms  execute <unnamed>:
	    WITH
	    latest_published_root_contents AS (
	        SELECT
	            contents.id,
	            contents.owner_id,
	            contents.parent_id,
	            contents.slug,
	            contents.title,
	            contents.status,
	            contents.source_url,
	            contents.created_at,
	            contents.updated_at,
	            contents.published_at,
	            contents.deleted_at,
	            get_current_balance('content:tabcoin', contents.id) as tabcoins
	        FROM contents
	        WHERE
	            parent_id IS NULL
	            AND status = 'published'
	            AND published_at > NOW() - INTERVAL '1 week'
	    ),
	    ranked_published_root_contents AS (
	        SELECT
	            *,
	            COUNT(*) OVER()::INTEGER as total_rows
	        FROM latest_published_root_contents
	        WHERE tabcoins > 0
	        ORDER BY
	            tabcoins DESC,
	            published_at DESC
	    ),
	    group_1 AS (
	        SELECT
	            *,
	            1 as rank_group
	        FROM ranked_published_root_contents
	        WHERE
	            published_at > NOW() - INTERVAL '36 hours'
	            AND tabcoins > 11
	        ORDER BY
	            published_at DESC
	        LIMIT 10
	    ),
	    group_2 AS (
	        SELECT * FROM group_1
	        UNION ALL
	        SELECT
	            *,
	            2 as rank_group
	        FROM ranked_published_root_contents
	        WHERE
	            published_at > NOW() - INTERVAL '24 hours'
	            AND tabcoins > 6
	            AND id NOT IN (SELECT id FROM group_1)
	        ORDER BY
	            rank_group,
	            published_at DESC
	        LIMIT 20
	    ),
	    group_3 AS (
	        (SELECT
	            *,
	            3 as rank_group
	        FROM ranked_published_root_contents
	        WHERE
	            published_at > NOW() - INTERVAL '12 hours'
	            AND id NOT IN (SELECT id FROM group_2)
	        ORDER BY
	            published_at DESC
	        LIMIT 5)
	        UNION ALL
	        SELECT * FROM group_2
	    ),
	    group_4 AS (
	        (SELECT
	            *,
	            4 as rank_group
	        FROM ranked_published_root_contents
	        WHERE
	            published_at > NOW() - INTERVAL '3 days'
	            AND tabcoins > 11
	            AND id NOT IN (SELECT id FROM group_3)
	        ORDER BY
	            published_at DESC
	        LIMIT 10)
	        UNION ALL
	        SELECT * FROM group_3
	    ),
	    group_5 AS (
	        (SELECT
	            *,
	            5 as rank_group
	        FROM ranked_published_root_contents
	        WHERE
	            published_at > NOW() - INTERVAL '72 hours'
	            AND tabcoins > 2
	            AND id NOT IN (SELECT id FROM group_4)
	        ORDER BY
	            published_at DESC
	        LIMIT 10)
	        UNION ALL
	        SELECT * FROM group_4
	    ),
	    ranked AS (
	        SELECT * FROM group_5
	        UNION ALL
	        SELECT
	            *,
	            6 as rank_group
	        FROM ranked_published_root_contents
	        WHERE id NOT IN (SELECT id FROM group_5)
	        ORDER BY
	            rank_group,
	            tabcoins DESC,
	            published_at DESC
	        LIMIT $1
	        OFFSET $2
	    )
	    SELECT
	        ranked.id,
	        ranked.owner_id,
	        ranked.parent_id,
	        ranked.slug,
	        ranked.title,
	        ranked.status,
	        ranked.source_url,
	        ranked.created_at,
	        ranked.updated_at,
	        ranked.published_at,
	        ranked.deleted_at,
	        ranked.tabcoins,
	        ranked.rank_group,
	        ranked.total_rows,
	        users.username as owner_username,
	        (WITH RECURSIVE children AS
	            (SELECT id,
	                 parent_id
	            FROM contents as all_contents
	            WHERE
	                all_contents.id = ranked.id
	                AND all_contents.status = 'published'
	            UNION ALL
	            SELECT
	                all_contents.id,
	                all_contents.parent_id
	            FROM contents as all_contents
	            INNER JOIN children ON all_contents.parent_id = children.id
	            WHERE all_contents.status = 'published'
	            )
	            SELECT count(children.id)::integer
	            FROM children
	            WHERE children.id NOT IN (ranked.id)
	        ) as children_deep_count
	        FROM ranked
	        INNER JOIN users ON ranked.owner_id = users.id
	        ORDER BY
	            rank_group,
	            tabcoins DESC,
	            published_at DESC;
DETAIL:  parameters: $1 = '30', $2 = '0'
duration: 3006.857 ms  execute <unnamed>: 
	      WITH content_window AS (
	      SELECT
	        COUNT(*) OVER()::INTEGER as total_rows,
	        id
	      FROM contents
	      WHERE contents.parent_id IS NOT DISTINCT FROM $3 AND contents.status = $4
	      ORDER BY contents.published_at DESC
	
	      LIMIT $1 OFFSET $2
	      )
	      
	      SELECT
	        contents.id,
	        contents.owner_id,
	        contents.parent_id,
	        contents.slug,
	        contents.title,
	        contents.body,
	        contents.status,
	        contents.source_url,
	        contents.created_at,
	        contents.updated_at,
	        contents.published_at,
	        contents.deleted_at,
	        users.username as owner_username,
	        content_window.total_rows,
	        get_current_balance('content:tabcoin', contents.id) as tabcoins,
	
	        -- Originally this query returned a list of contents to the server and
	        -- afterward made an additional roundtrip to the database for every item using
	        -- the findChildrenCount() method to get the children count. Now we perform a
	        -- subquery that is not performant but everything is embedded in one travel.
	        -- https://github.com/filipedeschamps/tabnews.com.br/blob/de65be914f0fd7b5eed8905718e4ab286b10557e/models/content.js#L51
	        (
	          WITH RECURSIVE children AS (
	            SELECT
	                id,
	                parent_id
	            FROM
	              contents as all_contents
	            WHERE
	              all_contents.id = contents.id AND
	              all_contents.status = 'published'
	            UNION ALL
	              SELECT
	                all_contents.id,
	                all_contents.parent_id
	              FROM
	                contents as all_contents
	              INNER JOIN
	                children ON all_contents.parent_id = children.id
	              WHERE
	                all_contents.status = 'published'
	          )
	          SELECT
	            count(children.id)::integer
	          FROM
	            children
	          WHERE
	            children.id NOT IN (contents.id)
	        ) as children_deep_count
	      FROM
	        contents
	      INNER JOIN
	        content_window ON contents.id = content_window.id
	      INNER JOIN
	        users ON contents.owner_id = users.id
	    
	      ORDER BY contents.published_at DESC
	      ;
DETAIL:  parameters: $1 = '30', $2 = '900', $3 = NULL, $4 = 'published'
duration: 2312.163 ms  execute <unnamed>: 
	      WITH content_window AS (
	      SELECT
	        COUNT(*) OVER()::INTEGER as total_rows,
	        id
	      FROM contents
	      WHERE contents.parent_id IS NOT DISTINCT FROM $3 AND contents.status = $4
	      ORDER BY contents.published_at DESC
	
	      LIMIT $1 OFFSET $2
	      )
	      
	      SELECT
	        contents.id,
	        contents.owner_id,
	        contents.parent_id,
	        contents.slug,
	        contents.title,
	        contents.body,
	        contents.status,
	        contents.source_url,
	        contents.created_at,
	        contents.updated_at,
	        contents.published_at,
	        contents.deleted_at,
	        users.username as owner_username,
	        content_window.total_rows,
	        get_current_balance('content:tabcoin', contents.id) as tabcoins,
	
	        -- Originally this query returned a list of contents to the server and
	        -- afterward made an additional roundtrip to the database for every item using
	        -- the findChildrenCount() method to get the children count. Now we perform a
	        -- subquery that is not performant but everything is embedded in one travel.
	        -- https://github.com/filipedeschamps/tabnews.com.br/blob/de65be914f0fd7b5eed8905718e4ab286b10557e/models/content.js#L51
	        (
	          WITH RECURSIVE children AS (
	            SELECT
	                id,
	                parent_id
	            FROM
	              contents as all_contents
	            WHERE
	              all_contents.id = contents.id AND
	              all_contents.status = 'published'
	            UNION ALL
	              SELECT
	                all_contents.id,
	                all_contents.parent_id
	              FROM
	                contents as all_contents
	              INNER JOIN
	                children ON all_contents.parent_id = children.id
	              WHERE
	                all_contents.status = 'published'
	          )
	          SELECT
	            count(children.id)::integer
	          FROM
	            children
	          WHERE
	            children.id NOT IN (contents.id)
	        ) as children_deep_count
	      FROM
	        contents
	      INNER JOIN
	        content_window ON contents.id = content_window.id
	      INNER JOIN
	        users ON contents.owner_id = users.id
	    
	      ORDER BY contents.published_at DESC
	      ;

E nosso maior inimigo é o RECURSIVE e eu especulo que todos esses logs irão sumir quando implementarmos algo como isso:

Fora isso, para certas coisas precisaremos rearquitetar a modelagem, como por exemplo, para o findChildrenTree() do content, vamos provavelmente adicionar uma coluna root_id e indexar tudo por ali.

@aprendendofelipe
Copy link
Collaborator

Fora isso, para certas coisas precisaremos rearquitetar a modelagem, como por exemplo, para o findChildrenTree() do content, vamos provavelmente adicionar uma coluna root_id e indexar tudo por ali.

Em se tratando de performance, também acho que criar o root_id deve ser a prioridade.

@FabricioFFC
Copy link
Contributor

FabricioFFC commented Mar 20, 2023

@aprendendofelipe e @filipedeschamps muito bom!

Vou focar essa semana nesse tópico, avaliando a criação do root_id. Embora antes dele, preciso aprofundar nas queries, para entender como a criação dele vai ajudar.

Obrigado pela ajuda de vcs!

@aprendendofelipe
Copy link
Collaborator

Show @FabricioFFC! 💪

Sobre a criação do root_id, a issue #1169 explica melhor, e se tiver qualquer dúvida a gente pode ir conversando lá. 🤝

Mas lá não é só otimizar a consulta, pois precisa modelar melhor os dados e mudar a regra de negócio, então se preferir lidar com os índices, também é algo que vai ajudar bastante. Eu só acho melhor começar por lá porque eliminar essa carga vai facilitar depois medir o impacto dos índices no desempenho. 👍

@FabricioFFC
Copy link
Contributor

Me parece também melhor começar pela #1169, pois a criação de índice só valeria agora se tivermos com uma dor latente em produção, que os índices aliviaria (não me parece ser o caso, se for me avise), já que a criação de índice provavelmente será impactada com a criação do root_id.

@aprendendofelipe
Copy link
Collaborator

@filipedeschamps, como estão os logs agora? Ainda está pegando algo acima de 2s?

@aprendendofelipe
Copy link
Collaborator

@filipedeschamps, como estão os logs agora? Ainda está pegando algo acima de 2s?

@filipedeschamps, quando for olhar os logs, aproveita e tenta ver se houve algum aumento significativo no uso do banco de dados com a mudança da revalidação da página de conteúdos de 10s para 1s. Subi para produção às 13:44 e parece que levou um tempo maior para diminuir o número de conexões abertas com o banco, mas já voltou para um patamar só um pouco acima de antes do deploy.

@aprendendofelipe
Copy link
Collaborator

Turma, os logs não vem registrando mais nenhuma query acima de 2s. E a utilização de CPU do banco dificilmente passa de 10%.

A gente pode diminuir o valor de log_min_duration_statement para descobrir outras queries que possam ser otimizadas, mas com essa utilização de CPU tão baixa, acho que podemos dar a issue o como concluída por enquanto, e voltar a analisar os índices/queries se algo piorar após a implementação da Revenue Share.

O que acham?

@Rafatcb
Copy link
Collaborator

Rafatcb commented Apr 11, 2024

Acho que faz sentido diminuir o valor do log_min_duration_statement para ficarmos cientes de quais são as próximas queries problemáticas, e também fechar este issue 👍

Então, se houver uma query significativamente lenta, acredito que podemos criar um issue dedicado para ela.

Você consegue ver como estão os endpoints? Pode ser que algum seja lento por executar várias queries separadas, e assim nenhuma query chama a atenção no log.

@aprendendofelipe
Copy link
Collaborator

Acho que faz sentido diminuir o valor do log_min_duration_statement para ficarmos cientes de quais são as próximas queries problemáticas,

Olhando os tempos médios de execução das funções lambdas, nenhuma query poderia estar passando de 1s. Podemos deixar para baixar o valor do log_min_duration_statement quando realmente precisar, ou aproveitamos quando formos fazer alguma outra alteração no terraform.

e também fechar este issue 👍

Vou fechar 🤝

Então, se houver uma query significativamente lenta, acredito que podemos criar um issue dedicado para ela.

No momento, não há! 🎉

Você consegue ver como estão os endpoints? Pode ser que algum seja lento por executar várias queries separadas, e assim nenhuma query chama a atenção no log.

Se fala dos tempos de resposta das requisições para cada endpoint, não temos essa métrica. Isso seria algo para ser coletado e enviado pelo client. Mas acredito que não fala disso, pois não seria muito útil para analisar a performance do banco de dados, já que boa parte das requisições são atendidas pelo cache.

Se fala do tempo de execução das lambdas, isso eu dei uma olhada. Não temos problemas de perfomance com as queries, mas, se fosse o caso de diminuir a utilização de recursos na Vercel (usamos cerca de 25% da cota mensal), deveríamos atacar nesta ordem:

  • GET /[username]/[slug]: só 290ms, mas impacta pelo volume de invocações, que chega a 10% da cota;
  • GET /api/v1/contents/rss: 1839ms, o que faz chegar a 4% da cota, e o problema não é o BD;
  • GET /api/v1/contents/[username]/[slug]/thumbnail: só 390ms, mas impacta pelo volume, que chega a 3% da cota;
  • Outras listagens de conteúdos, excluindo o RSS, chegam juntas a 2% da cota que temos na Vercel.

Agora, olhando para UX, precisamos atacar nesta ordem:

  • GET /api/v1/contents/rss: 1839ms, pois puxa mais dados do banco (contém o body), precisa remover o markdown para criar a description, gerar html para o content, filtrar a saída com o validator, e tudo isso para 30 conteúdos.
  • POST /api/v1/contents: 1514ms, pois faz muitas leituras e gravações no banco (talvez dê para unificar algumas);

O RSS só fica no topo aqui porque eu desconsiderei todos os POSTs/PATCHs que envolvem salvar ou verificar a senha, pois são os mais lentos por causa do bcryptjs.

Por fim, olhando para SEO, acho que o foco seria:

  • GET /api/v1/contents/rss: pois é acessado com frequência pelo Google, e o P99 passa de 8s;
  • GET /api/v1/contents/[username]/[slug]/thumbnail: pois é acessado por diferentes redes sociais, e o P99 chega a 4s;

Com tudo isso, se for para dar atenção para algum endpoint, começaria pelo RSS.

Mas nada disso envolve problemas com alguma query específica, mas sim com o que é executado na lambda, então de fato essa issue está concluída. 🎉🎉🎉

@Rafatcb
Copy link
Collaborator

Rafatcb commented Apr 13, 2024

Se fala do tempo de execução das lambdas

Era isso mesmo. Obrigado pelos dados 🤝, excelente resumo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back Envolve modificações no backend desempenho Melhoria de desempenho
Projects
None yet
Development

No branches or pull requests

4 participants